Skip to content

Code quality improvements for the global styles endpoint#36071

Merged
youknowriad merged 1 commit intotrunkfrom
update/global-styles-endpoitns
Oct 29, 2021
Merged

Code quality improvements for the global styles endpoint#36071
youknowriad merged 1 commit intotrunkfrom
update/global-styles-endpoitns

Conversation

@youknowriad
Copy link
Copy Markdown
Contributor

Address follow-up comments in #35801

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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