Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code quality improvements for the global styles endpoint #36071

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

youknowriad
Copy link
Contributor

Address follow-up comments in #35801

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine and code looks good.

@oandregal
Copy link
Member

For context, I've noticed the following. I presume it has to do with how core data works but I'm unfamiliar with that, so I thought I'd share:

I've noticed there are two GET requests when the site editor is first loaded: http://wordpressnightly.local/wp-json/wp/v2/global-styles/87?_locale=user and http://wordpressnightly.local/wp-json/wp/v2/global-styles/87?context=edit&_locale=user.

Then the first time I click something in the GS sidebar (tested changing the line height or changing the background color) there's a new GET request to http://wordpressnightly.local/wp-json/wp/v2/global-styles/87?context=edit&_locale=user.

Finally, there's a POST to http://wordpressnightly.local/wp-json/wp/v2/global-styles/87?_locale=user when saving the data.

@youknowriad
Copy link
Contributor Author

I've noticed there are two GET requests when the site editor is first loaded: http://wordpressnightly.local/wp-json/wp/v2/global-styles/87?_locale=user and http://wordpressnightly.local/wp-json/wp/v2/global-styles/87?context=edit&_locale=user.

Yes, that is unfortunately needed for now as right now we only receive the url as a "link" from the themes endpoint to the current user styles, so basically we just call that request to retrieve the "ID" and use the entities later. I preferred doing this over trying to "parse" the URL and extract the "id".

@youknowriad youknowriad merged commit 2d00327 into trunk Oct 29, 2021
@youknowriad youknowriad deleted the update/global-styles-endpoitns branch October 29, 2021 11:17
@github-actions github-actions bot added this to the Gutenberg 11.9 milestone Oct 29, 2021
@spacedmonkey
Copy link
Member

@youknowriad Can you please stop merging PRs before I or a member of the REST API team get a chance to even look at them. This was merged with 3 hours of it's creation, not giving me any time to review.

As mentioned, the post controller is the best way forward, so I have created a new PR here #36076. This endpoint needs unit tests ASAP if it is going to get into core.

@youknowriad
Copy link
Contributor Author

Last I saw there was a disagreement between you and @TimothyBJacobs on whether the post controller is the best path forward, so feel free to continue the iterations there and we can make merge it once it's ready and approved.

For this PR, this was just a small PR addressing your own suggestions, so I don't see why I should have waited once it's approved by another core member.

There are teams, but we're all core committers and contributors here :) no need for drama.

@spacedmonkey
Copy link
Member

spacedmonkey commented Oct 30, 2021

@youknowriad Not everyone is full time on this project. Not giving contributors time to review your work, means that PRs are not correctly reviewed and mistakes and errors get merged. It is disrespectful not to give us this time. It is basically side stepping the code review process. I asked nicely, please wait for more time for feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants