-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update Global Styles endpoint to support non-experimental paths #16823
Conversation
You can trigger an installable build for these changes by visiting CircleCI here. |
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
DDLogError("Error fetching editor settings: \(err)") | ||
// The user may not have the gutenberg plugin installed so try /wp/v2/themes to maintain feature support. | ||
// In WP 5.9 we may be able to skip this attempt. | ||
self.fetchTheme(completion) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrying on error should be fine. I wonder if we can add an extra check for the specific error type (404
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing specific here but I wasn't sure of about all of the ways this could fail so I decided to play it safe.
|
||
// The app will call the none-experimental path first but fail because of compatibility reasons | ||
mockOrgRemoteApi.completionPassedIn!(.failure(NSError(domain: "test", code: 404, userInfo: nil)), HTTPURLResponse()) | ||
// The app will then retry the experimental path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test looks great. We just need to update the comment to reflect the removal of the experimental path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout I missed this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @chipsnyder 👍
Tested this on an iPhone SE 2020 (iOS 14.4.1) and everything works as expected. The code LGTM 🎉
I just left two minor comments.
This updates the Global styles endpoint to use the non-experimental paths for fetching the block editor settings.
Related PR wordpress-mobile/WordPressKit-iOS#416
To test:
For each of these cases, it'll be enough to open the editor and validate that the correct color options are available. It would also be useful to test using a network monitoring tool to make sure the correct paths are called. This will help ensure you aren't seeing cached data if you're reusing the same site for testing.
/wp-block-editor/v1
/wp-block-editor/v1
which will fail, so the fallback ofwp/v2/themes
should be used/wp-block-editor/v1
/wp-block-editor/v1
=Regression Notes
Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
What automated tests I added (or what prevented me from doing so)
Update unit tests to validate each flow
PR submission checklist:
RELEASE-NOTES.txt
if necessary.