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

Improve AMP admin bar item when DevTools is turned on or off #4986

Merged
merged 14 commits into from
Jul 6, 2020
Merged

Improve AMP admin bar item when DevTools is turned on or off #4986

merged 14 commits into from
Jul 6, 2020

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jul 4, 2020 •
edited
Loading

Summary

This is a follow-up to #4955 (for #2673), specifically this line:

And the admin bar simply shows a link back to the non-AMP version, with no Validate link, CSS usage info, or paired browsing (aside: the link text is not entirely intuitive here)

Before this PR, when DevTools is turned off, on an non-AMP page the admin bar item contains "🔗 AMP" and on the non-AMP page it contains "🔗 Non-AMP". This is confusing because it's not clear whether the icon is referring to the page being non-AMP or the link being to the AMP version:

State Before on non-AMP page Before on AMP page
None image image
Hover image image

This PR improves that situation by:

  1. Changing the 🔗 icon to the ⚡ AMP logo when on an AMP page.
  2. Keeping the admin menu item text just “AMP”.
  3. Adding a submenu item that explicitly says, “View non-AMP version” when on an AMP page, and “View AMP version” when on a non-AMP page.
State After on non-AMP page After on AMP page
None image image
Hover image image

Also, when DevTools are enabled, when there are no validation errors on a page, now instead of saying “Validate 0 issues“ the link text will remain just “Validate”:

Before After
image image

Additionally, when there are are validation errors now they will be explicitly mentioned. The small element is used to reduce the width of the menu item.

All reviewed:

image

Some unreviewed (all others removed and reviewed):

image

Some kept and some unreviewed (in a paired mode):

image

Some kept without any unreviewed (in a paired mode):

image

And now, in Standard mode when there is kept markup that is reviewed, a warning icon is shown instead of green checkmark:

image

All kept:

image

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

@westonruter westonruter added this to the v1.6 milestone Jul 4, 2020
@googlebot googlebot added the cla: yes Signed the Google CLA label Jul 4, 2020
@github-actions
Copy link
Contributor

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

Plugin builds for 3d5b5c4 are ready 🛎️!

pierlon
pierlon reviewed Jul 5, 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.

I tend to prefer this being the other way around: the ⚡ AMP logo being used to link to the AMP page with the text "View AMP", and vice-versa with the 🔗 icon being used to link to the non-AMP page with the text "View non-AMP". The submenu seems unnecessary if no other submenus are there.

Non-AMP page:
image

AMP page:
image

@westonruter
Copy link
Member Author

I tend to prefer this being the other way around: the ⚡ AMP logo being used to link to the AMP page with the text "View AMP", and vice-versa with the 🔗 icon being used to link to the non-AMP page with the text "View non-AMP". The submenu seems unnecessary if no other submenus are there.

I can see that, but this is the reverse of the behavior when DevTools is turned on. Also, using the 🔗 icon when on the non-AMP version and the AMP logo when on the AMP version is closer aligned to what the AMP Validator extension does. Note the consistency with the changes in this PR:

Context Non-AMP AMP
AMP Validator Extension image image
Admin bar with DevTools turned off image image
Admin bar with DevTools Turned on image image

Also, keeping the parent admin bar menu item to just “AMP” takes up less space which can be limited with other plugins adding their own admin menu bar items.

We could also have a custom SVG for the AMP 🔗 icon, but that is somewhat redundant since it already says “AMP”. If we did that, however, then we could eliminate the “AMP” text. But then we'd have to figure out what to do when DevTools is turned on, some AMP logo with a warning overlay perhaps, like the validator extension does:

image image image image

This is similar to what we discussed before when the icons were changed from emoji in the first place: #3726 (comment)

@pierlon
Copy link
Contributor

pierlon commented Jul 5, 2020

Thanks for taking the time to explain the reasoning behind the changes in detail. Really appreciate it 🙇.

amedina
amedina approved these changes Jul 6, 2020
View reviewed changes
Copy link
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

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

This a great changes and a resulting much better UX.

A bit of calibration for the Validate link as discussed.

@westonruter
Update 'Validate' admin bar item link to 'Validate URL'
c188f4a
@westonruter
Show warning in admin bar when kept invalid markup in standard mode
866f180
@westonruter
Improve displaying error counts in validate link
36a7846
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into improve/n…
60cbee6 
…on-devtools-admin-bar-item

* 'develop' of github.com:ampproject/amp-wp: (32 commits)
  Use empty() instead of isset()
  Fix lint issue
  Set mobile_redirect to true from `originalOptions` when value is unchanged in first run
  Remove previously completed todo
  Optimize AMP_Tag_And_Attribute_Sanitizer::get_missing_mandatory_attributes()
  Run astra theme e2e tests separately so theme can be activated/removed only once
  Recommend transitional/reader mode to technical users with amp-compatible active theme
  Fix Jest tests
  Onboarding wizard close link: return to previous location
  Persist default mobile_redirect setting when it is not interacted with
  Prevent wpfooter from preventing clicks
  Fix lint issues
  Change currentThemeIsReaderTheme to currentThemeIsAmongReaderThemes
  Update Browse/Customize AMP text and placement on done screen
  Determine is_reader_theme in PHP instead of JS
  Break up long import statement into multiple lines
  Comment typo fix
  Restore blocking mobile switcher link if in Customizer but only for Reader mode
  Eliminate blocking of mobile version link switcher in customizer preview
  Allow mobile redirection in dev mode generally; improve paired browsing detection
  ...
@westonruter
Copy link
Member Author

@pierlon @amedina Please re-review as I've made improvements to how the validation errors are listed in the link, and now the warning icon is used in Standard mode when there are unreviewed errors or there are kept ones.

https://github.com/ampproject/amp-wp/pull/4986/files/65c9e10..36a7846

@westonruter westonruter merged commit 3a85a38 into develop Jul 6, 2020
@westonruter westonruter deleted the improve/non-devtools-admin-bar-item branch July 6, 2020 23:39
@westonruter westonruter changed the title Improve AMP admin bar item when DevTools is turned off Improve AMP admin bar item when DevTools is turned on or off Jul 6, 2020
@amedina
Copy link
Member

amedina commented Jul 15, 2020

QA passed

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 18, 2020
@westonruter westonruter mentioned this pull request Aug 5, 2020
Closed
@westonruter westonruter added the WS:Core Work stream for Plugin core label Aug 11, 2020
@westonruter westonruter mentioned this pull request Sep 14, 2020
Closed
@westonruter westonruter mentioned this pull request Apr 16, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amedina amedina amedina approved these changes

@pierlon pierlon pierlon approved these changes

Assignees
No one assigned
Labels
Changelogged Whether the issue/PR has been added to release notes. cla: yes Signed the Google CLA WS:Core Work stream for Plugin core
Projects
None yet
Milestone
v2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@westonruter @pierlon @amedina @googlebot