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.

Sign up for GitHub

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

Jump to bottom

Supported post types/templates setting: move to React/REST #5044

Merged
merged 55 commits into from
Jul 16, 2020
Merged

Supported post types/templates setting: move to React/REST #5044

merged 55 commits into from
Jul 16, 2020

Conversation

johnwatkins0
Copy link
Contributor

@johnwatkins0 johnwatkins0 commented Jul 14, 2020 •
edited by westonruter
Loading

Summary

Fixes #5015
Fixes #5046

This fixes #5015 by converting the PHP-based interface for managing supporting post types/templates on the Settings page to React. It should work the same as before from a user perspective. Data is now saved over REST, and the settings page no longer needs to refresh to save the data. Instead we now show a "Settings saved" notice on the screen for several seconds.

Things to test

  • Conditional showing/hiding of various pieces of the interface depending on the active theme mode and whether the "Serve all templates" toggle is on/off
  • Data saves correctly
  • Any minor non-blocking UI feedback can go in Minor UI updates on settings screen #5018, which I'm working on today.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@googlebot googlebot added the cla: yes Signed the Google CLA label Jul 14, 2020
@johnwatkins0 johnwatkins0 self-assigned this Jul 15, 2020
Base automatically changed from feature/5014-react-plugin-suppression to develop July 15, 2020 04:11
@johnwatkins0 johnwatkins0 force-pushed the feature/5015-react-post-template-settings branch from 73c933f to c125be1 Compare July 15, 2020 04:40
@johnwatkins0 johnwatkins0 force-pushed the feature/5015-react-post-template-settings branch from ed311e1 to 9e129f9 Compare July 15, 2020 13:22
@johnwatkins0 johnwatkins0 marked this pull request as ready for review July 15, 2020 14:46
@github-actions
Copy link
Contributor

github-actions bot commented Jul 15, 2020 •
edited
Loading

Plugin builds for a8fd8a8 are ready 🛎️!

westonruter
westonruter requested changes Jul 15, 2020
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

@johnwatkins0 I've selected a Reader theme but the template selection is not available to me:

image

Also, when I've selected Legacy, in which it is correct to hide the template selection, the toggle does not appear as disabled:

image

I wonder if we should just hide the “Serve all templates as AMP” altogether in this case. The notice is confusing because the section as a whole is relevant to Legacy mode whereas the notice is supposed to only be for the toggle.

@johnwatkins0
Copy link
Contributor Author

I wonder if we should just hide the “Serve all templates as AMP” altogether in this case. The notice is confusing because the section as a whole is relevant to Legacy mode whereas the notice is supposed to only be for the toggle.

I think this is the right approach. I'm going to try it (while fixing the other thing).

@johnwatkins0
Copy link
Contributor Author

I wonder if we should just hide the “Serve all templates as AMP” altogether in this case. The notice is confusing because the section as a whole is relevant to Legacy mode whereas the notice is supposed to only be for the toggle.

I think this is the right approach. I'm going to try it (while fixing the other thing).

Updated in f27fec7

pierlon
pierlon suggested changes Jul 15, 2020
View reviewed changes
assets/src/components/user-context-provider/index.js Outdated Show resolved Hide resolved
assets/src/settings-page/supported-templates.js Show resolved Hide resolved
src/OptionsRESTController.php Outdated Show resolved Hide resolved
johnwatkins0 and others added 9 commits July 15, 2020 18:47
@johnwatkins0
Add separate error handling for errors not inside an error boundary
08e8315
@johnwatkins0
Merge branch 'feature/5015-react-post-template-settings' of https://g…
1293775 
…ithub.com/ampproject/amp-wp into feature/5015-react-post-template-settings
@westonruter @pierlon
Use static
1ce99f0 
Co-authored-by: Pierre Gordon <[email protected]>
@westonruter @pierlon
Simplify import path
65503ea 
Co-authored-by: Pierre Gordon <[email protected]>
@johnwatkins0
Update snapshot
1ea4ba2
@westonruter
Fix typo and improve variable naming
d196d91
@johnwatkins0
Remove snapshot test that wasn't useful
ac8cab4
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into feature/5…
8786ed6 
…015-react-post-template-settings

* 'develop' of github.com:ampproject/amp-wp:
  Bump lodash from 4.17.15 to 4.17.19
@johnwatkins0
Merge branch 'feature/5015-react-post-template-settings' of https://g…
f442dc8 
…ithub.com/ampproject/amp-wp into feature/5015-react-post-template-settings
@westonruter
Copy link
Member

(I asked @pierlon to investigate the JS unit test failure introduced by merging #5052.)

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Jul 16, 2020
@pierlon
Copy link
Contributor

pierlon commented Jul 16, 2020

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Jul 16, 2020
@johnwatkins0 johnwatkins0 mentioned this pull request Jul 16, 2020
Merged
9 tasks
@westonruter westonruter mentioned this pull request Jul 16, 2020
Merged
3 tasks
pierlon
pierlon approved these changes Jul 16, 2020
View reviewed changes
Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

Approving since the failing Optimizer tests are unrelated to the PR.

@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into feature/5…
a8fd8a8 
…015-react-post-template-settings

* 'develop' of github.com:ampproject/amp-wp:
  Ignore write-only property errors
  Ignore unused private message errors
westonruter
westonruter approved these changes Jul 16, 2020
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

😍

@westonruter westonruter added this to the v1.6 milestone Jul 16, 2020
@westonruter westonruter merged commit a890857 into develop Jul 16, 2020
@westonruter westonruter deleted the feature/5015-react-post-template-settings branch July 16, 2020 05:59
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020
@johnwatkins0 johnwatkins0 added the WS:UX Work stream for UX/Front-end label Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierlon pierlon pierlon approved these changes

@westonruter westonruter westonruter approved these changes

@schlessera schlessera Awaiting requested review from schlessera

Assignees

@johnwatkins0 johnwatkins0

Labels
Changelogged Whether the issue/PR has been added to release notes. cla: yes Signed the Google CLA WS:UX Work stream for UX/Front-end
Projects
None yet
Milestone
v2.0
4 participants
@johnwatkins0 @westonruter @googlebot @pierlon