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

Update Settings page to include "Other" section #6501

Merged
merged 30 commits into from
Sep 13, 2021

Conversation

delawski
Copy link
Collaborator

@delawski delawski commented Jul 30, 2021

Summary

Fixes #5578

This PR removes the Mobile redirection toggle from the Onboarding Wizard and adds it to a new "Other" section on the AMP Settings page. The "Other" section also contains a Dev Tools toggle:

Screenshot 2021-08-20 at 14 56 49

Checklist

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

@delawski delawski added the WS:UX Work stream for UX/Front-end label Jul 30, 2021
@delawski delawski added this to the v2.2 milestone Jul 30, 2021
@delawski delawski requested review from westonruter and pierlon July 30, 2021 15:37
@github-actions
Copy link
Contributor

github-actions bot commented Jul 30, 2021

Plugin builds for 4b3a89a are ready 🛎️!

@delawski delawski changed the title Feature/5578 add new settings Update Settings page to include "Other" section Jul 30, 2021
@delawski
Copy link
Collaborator Author

I've been trying to fix the failing E2E test but ran out of time. I'm pretty sure there's an easy fix for that.

@pierlon
Copy link
Contributor

pierlon commented Jul 30, 2021

I've been trying to fix the failing E2E test but ran out of time. I'm pretty sure there's an easy fix for that.

No worries. I'll take a look.

@delawski
Copy link
Collaborator Author

No worries. I'll take a look.

Thanks so much, @pierlon!

@CLAassistant
Copy link

CLAassistant commented Jul 30, 2021

CLA assistant check
All committers have signed the CLA.

@delawski delawski force-pushed the feature/5578-add-new-settings branch 2 times, most recently from 7f6e93b to 0422246 Compare August 16, 2021 15:33
@delawski
Copy link
Collaborator Author

@jwold Thanks for updating the component!

@pierlon @westonruter I've been able to fix the E2E tests, increase the test coverage, and also resolve the merge conflicts. Now the PR is "green" and I think it's good to go through the final review.

<section className="developer-tools">
<DevToolsToggle />
<p>
{ __( 'Only enabled for your user account. This is not a sitewide setting.', 'amp' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also mention what it's useful for, similar to how it was on the User Profile page:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about something like this?

Screenshot 2021-08-19 at 18 30 41

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is addressed in 753175f

@delawski delawski force-pushed the feature/5578-add-new-settings branch from 753175f to 2f7d0b5 Compare September 3, 2021 09:30
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.

Great work @delawski. Passing onto @westonruter for a final review.

…578-add-new-settings

* 'develop' of github.com:ampproject/amp-wp: (44 commits)
  Make placeholder dependency on video ID more explicit
  Fix adding start to placeholder link; improve coverage
  Let placeholder link point to YouTube permalink and not iframe src
  Prevent generating closing IMG tags
  Use coversDefaultClass
  Remove condition which cannot be reached since video ID is always provided
  Opt to pass-through iframe for unrecognized YouTube URL rather than convert to link
  Remove unreachable code
  Give more explicit array phpdoc type
  Fix issues identified by phpstan
  Try installing phpstan via shivammathur/setup-php
  Try downloading a specific version of phpstan instead of latest
  Fix yml syntax error
  Try removing phpstan before composer install
  Print phpstan version when running on GHA
  Revert "Revert phpstan ignores"
  Try adding phpstan version to composer cache key
  Revert phpstan ignores
  update unit test cases
  Sanitize attribute name Use original YouTube URL as placeholder when sanitizing raw embed Fix phpcs alignment warning Use constant instead of member variable Let Document::fromNode() obtain the ownerDocument Deduplicate iframe_props into constant
  ...
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.

I added long-missing tests for OptionsMenu which I've added in 121ca24.

One of the requirements was:

Remove "redirect mobile visitors to AMP" toggle from onboarding

I've implemented this in ba02cab.

The main thing left is the E2E tests being fixed, aside from the minor comments I'm leaving here.

* @param {string} props.userRestPath REST endpoint to retrieve options.
* @param {Object} props Component props.
* @param {?any} props.children Component children.
* @param {boolean} props.allowConfiguredPluginOnly Provided only for functionality that requires the plugin to be configured.
Copy link
Member

Choose a reason for hiding this comment

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

This prop name and description are unclear to me. Can this be clarified?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the description was totally wrong and the prop name could be better too.

In 418ec3c I've changed those. I'm still not convinced it's clear enough but maybe you can find a better wording?

To make it clearer, I've also replaced the REST route we used: /wp/v2/users/me with a general users path: /wp/v2/users. This way, the prop name (usersResourceRestPath) should now semantically match its value.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see that allowConfiguredPluginOnly was changed.

The users REST path change looks good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh my! I have no idea why I looked at the wrong prop (and "fixed" it). I'll get the allowConfiguredPluginOnly clarified.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this flag is used to allow fetching users data even if the plugin is not fully configured (the Onboarding Wizard has not been completed).

So far, the user data was fetched only after completing the Wizard but right now we need the user data earlier (it's possible to visit the AMP Settings page without going through the Wizard).

Naming is hard. Anything I come up with so far is long and may sound a bit awkward: skipFetchingIfUnconfiguredPlugin, onlyFetchIfPluginIsConfigured... Suggestions are well appreciated.

Copy link
Member

Choose a reason for hiding this comment

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

onlyFetchIfPluginIsConfigured sounds good to me!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perfect! It's been updated in 4b3a89a.

@westonruter
Copy link
Member

Oh, hold on, E2E tests are passing now! 🤷

Maybe they'll fail again when the above suggestions are applied.

@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #6501 (aafb4b1) into develop (deba08a) will increase coverage by 0.43%.
The diff coverage is 89.47%.

❗ Current head aafb4b1 differs from pull request most recent head 418ec3c. Consider uploading reports for the commit 418ec3c to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #6501      +/-   ##
=============================================
+ Coverage      75.83%   76.26%   +0.43%     
  Complexity      6060     6060              
=============================================
  Files            190      240      +50     
  Lines          18219    19001     +782     
=============================================
+ Hits           13817    14492     +675     
- Misses          4402     4509     +107     
Flag Coverage Δ
javascript 78.84% <66.66%> (?)
php 76.15% <100.00%> (+0.32%) ⬆️
unit 76.15% <100.00%> (+0.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
assets/src/components/redirect-toggle/index.js 75.00% <0.00%> (ø)
...sets/src/onboarding-wizard/components/nav/index.js 100.00% <ø> (ø)
assets/src/components/dev-tools-toggle/index.js 80.00% <80.00%> (ø)
src/Admin/OptionsMenu.php 89.09% <100.00%> (+53.90%) ⬆️
src/MobileRedirection.php 83.51% <100.00%> (ø)
assets/src/components/svg/transitional.js 100.00% <0.00%> (ø)
...idation/components/error/get-error-source-title.js 93.75% <0.00%> (ø)
.../block-validation/hooks/use-amp-document-toggle.js 80.00% <0.00%> (ø)
...ock-validation/components/error/error-type-icon.js 100.00% <0.00%> (ø)
...ets/src/block-validation/components/error/index.js 100.00% <0.00%> (ø)
... and 46 more

@delawski
Copy link
Collaborator Author

Oh, hold on, E2E tests are passing now! 🤷

Maybe they'll fail again when the above suggestions are applied.

That's strange. I don't seem to be getting the E2E error locally. I think this may be some kind of hazard where the Puppeteer doesn't wait for a specific selector before assessing its existence.

@westonruter westonruter merged commit 6164a77 into develop Sep 13, 2021
@westonruter westonruter deleted the feature/5578-add-new-settings branch September 13, 2021 19:37
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021
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. WS:UX Work stream for UX/Front-end
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update AMP settings to include additional options
5 participants