Skip to content

Settings screen updates#4998

Merged
westonruter merged 100 commits intodevelopfrom
feature/4704-settings-screen-updates
Jul 10, 2020
Merged

Settings screen updates#4998
westonruter merged 100 commits intodevelopfrom
feature/4704-settings-screen-updates

Conversation

@johnwatkins0
Copy link
Copy Markdown
Contributor

@johnwatkins0 johnwatkins0 commented Jul 7, 2020 •
edited
Loading

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).

johnwatkins0 added 18 commits July 5, 2020 20:31
@johnwatkins0
Remove settings screen markup
a23e0b4
@johnwatkins0
Refactor PHP classes for dependecy injection, update tests, setup adm…
a9e3d07 
…in screen
@johnwatkins0
Fix tests
9974b5c
@johnwatkins0
Welcome section
0eea9d5
@johnwatkins0
Merge branch 'feature/4974-remove-close-on-finish-screen' into featur…
5192f62 
…e/4704-settings-screen-updates
@johnwatkins0
Settings screen parts
398ef55
@johnwatkins0
Merge branch 'feature/4974-remove-close-on-finish-screen' into featur…
c0d28d0 
…e/4704-settings-screen-updates
@johnwatkins0
Start on supported templates component
4288269
@johnwatkins0
Merge remote-tracking branch 'origin/develop' into feature/4704-setti…
209222d 
…ngs-screen-updates
@johnwatkins0
Complete working settings screen without tests or reader themes
b9a3aa1
@johnwatkins0
PHPUnit tests
aa32c6f
@johnwatkins0
Rename test class
ce2ad8b
@johnwatkins0
Update e2e tests with minor fixes
fe68e6b
@johnwatkins0
Merge remote-tracking branch 'origin/develop' into feature/4704-setti…
336f684 
…ngs-screen-updates
@johnwatkins0
Integrate reader theme seleection on settings page
a16b9cf
@johnwatkins0
Resolve minor issues
fdba128
@johnwatkins0
Linting fixes
4ed8e50
@johnwatkins0
Improve tests
e19f4c1
@googlebot googlebot added the cla: yes Signed the Google CLA label Jul 7, 2020
@johnwatkins0
Copy link
Copy Markdown
Contributor Author

johnwatkins0 commented Jul 7, 2020

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.

@johnwatkins0 johnwatkins0 requested a review from westonruter July 7, 2020 23:21
pierlon
pierlon suggested changes Jul 9, 2020
View reviewed changes
includes/class-amp-theme-support.php
}
if ( ! isset( $support[0] ) || ! is_array( $support[0] ) ) {
return [];
return [ 'paired' => false ];
Copy link
Copy Markdown
Contributor

@pierlon pierlon Jul 9, 2020

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
Copy Markdown
Contributor Author

@johnwatkins0 johnwatkins0 Jul 9, 2020

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

@westonruter westonruter Jul 9, 2020

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.

@johnwatkins0 johnwatkins0 mentioned this pull request Jul 9, 2020
Closed
This was referenced Jul 9, 2020
Closed
Closed
westonruter
westonruter reviewed Jul 9, 2020
View reviewed changes
westonruter
westonruter reviewed Jul 9, 2020
View reviewed changes
westonruter
westonruter approved these changes Jul 9, 2020
View reviewed changes
Copy link
Copy Markdown
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
pierlon
pierlon suggested changes Jul 9, 2020
View reviewed changes
@westonruter @pierlon
Improve formatting of active theme notice
f717920 
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
pierlon
pierlon approved these changes Jul 9, 2020
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@westonruter westonruter westonruter approved these changes

@jwold jwold Awaiting requested review from jwold

@schlessera schlessera Awaiting requested review from schlessera

+1 more reviewer

@pierlon pierlon pierlon approved these changes

Reviewers whose approvals may not affect merge requirements

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

6 participants

@johnwatkins0 @jwold @westonruter @schlessera @pierlon @googlebot