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

Settings screen updates #4998

Merged
merged 100 commits into from Jul 10, 2020
Merged

Conversation

johnwatkins0
Copy link
Contributor

@johnwatkins0 johnwatkins0 commented Jul 7, 2020

Summary

Fixes #4475
Fixes #4990
Fixes #4560
Fixes #4704

This revamps the main plugin settings screen using components from the setup wizard. A lot of this change set is moving files around and renaming things. Mainly, a few related PHP classes were moved into the services architecture, while some of the wizard components were moved up a level in the file system and refactored in small ways as needed to support both purposes.

Updates to test/review on the settings screen:

  1. The wizard_completed option has been changed to plugin_configured. It's now set to true when the user completes the wizard or saves the settings screen.
  2. When plugin_configured is false, the welcome area at the top urges the user to go to the wizard. When it's true, the text changes.
  3. When plugin_configured is false and there is a suggested mode (or built-in support) provided by the active theme, we put an info notice on that mode. I think further enhancements will be needed around this.
  4. When Reader is chosen but the theme's suggested mode is one of the others, we display a warning notice.
  5. When the active theme suggests Reader mode, we indicate that as well.
  6. When the active theme requires all templates to support AMP, we show a notice in place of the supported templates toggle.
  7. When the active theme is a reader theme, we disable that theme from the reader themes list and show a notice (in contrast to the way we handle it in the wizard, which is to automatically switch to transitional if they choose their active theme).
  8. Various bits of conditional section toggling occur based on selected options. Rather than trying to describe them here, I think they just need to be tested to make sure they make sense.

Not included

Two parts of the settings screen are still rendering on the backend: the template-level support checkboxes and the plugin suppression table. Both of these features can be done on the React side, but there wasn't going to be time in this update, so we now wrap them in the new components and styles and hit form.submit() to save them and refresh the page after the REST/React-based options have finished saving.

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 7, 2020
@johnwatkins0
Copy link
Contributor Author

Tests are passing on this and it's mostly ready. I'm keeping it in draft status for the moment because there are a couple pieces I want to go over this evening and maybe tomorrow morning, but anyone who wants to check it out and offer feedback can do so any time.

assets/src/components/template-mode-option/index.js Outdated Show resolved Hide resolved
@@ -269,9 +269,10 @@ public static function get_theme_support_args() {
];
}
if ( ! isset( $support[0] ) || ! is_array( $support[0] ) ) {
return [];
return [ 'paired' => false ];
Copy link
Contributor

Choose a reason for hiding this comment

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

The intent here is if I say add_theme_support( 'amp' ) that implies my theme would support Standard mode, right?

cc @westonruter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is my understanding.

Copy link
Member

Choose a reason for hiding this comment

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

Correct. In practice, however, I think this flag is not useful. In #5010 I'm going to propose that we make paired => true the default instead.

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.

Great work! Impressive how you were able to shim the old and the new into the same UI.

@westonruter westonruter requested a review from pierlon July 9, 2020 22:56
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
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.

Really great job done here. The settings screen finally looks and feels modern ❤️.

@westonruter westonruter merged commit 01202bb into develop Jul 10, 2020
@westonruter westonruter deleted the feature/4704-settings-screen-updates branch July 10, 2020 00:01
@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
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
6 participants