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 font sizes in onboarding wizard and settings screen #5192

Merged
merged 21 commits into from
Aug 8, 2020
Merged

Improve font sizes in onboarding wizard and settings screen #5192

merged 21 commits into from
Aug 8, 2020

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Aug 6, 2020 •
edited
Loading

Summary

  • Reduce font-size of heading in theme cards to 1rem.
  • Set font-size of h3 to consistently be 1.2rem regardless of viewport width.
Before After
before-resized after-resized

👇 See comments below for additional changes.

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 v2.0 milestone Aug 6, 2020
@westonruter westonruter requested a review from jwold August 6, 2020 19:52
@westonruter westonruter marked this pull request as ready for review August 6, 2020 19:52
@google-cla google-cla bot added the cla: yes Signed the Google CLA label Aug 6, 2020
@westonruter westonruter requested a review from amedina August 6, 2020 19:55
@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2020 •
edited
Loading

Plugin builds for dd7e592 are ready 🛎️!

amedina
amedina approved these changes Aug 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.

Very nice in terms proportionality and consistency. Ship it!

@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into fix/font-…
492fd87 
…sizes

* 'develop' of github.com:ampproject/amp-wp: (22 commits)
  Allow legacy theme name to be translated, update description, use constant, s/classic/legacy/g
  Reindex array keys to ensure they are sequential
  Remove unused import and unskip tests
  Fix equals sign alignment
  Make sure override doesn't block selection in wizard
  Remove unnecessary init call on wp_scripts instance
  Mark action as internal
  Settings screen: switch to legacy if selected reader theme is active site theme
  Include JS maps if it's a development build
  Ensure JS vendor directory is included in build
  Call amp_register_polyfills action in a few more places and document it
  Add action hook for registration of polyfills
  Use curly-brace substitution instead of sprintf/concatenation
  Comment spacing fix
  Polyfill assets during paired browsing
  Remove conditional and delayed interfaces from polyfills class
  Modify conditional check for class registration
  Polyfill assets during paired browsing
  Initiate scripts and styles in tests
  Remove lodash from bundles (shouldn't have been included in the first place)
  ...
@westonruter
Prevent horizontal scrolling for analytics code sample and show trash…
d1f36ea 
… icon on mobile
@westonruter
Copy link
Member Author

westonruter commented Aug 6, 2020 •
edited
Loading

A couple more fixes for Analytics:

Before After
before-analytics after-analytics-2

@westonruter
Copy link
Member Author

Also improved Plugin Suppression on mobile viewport:

Before After
before-plugin-suppression after-plugin-suppression

@jwold
Copy link
Collaborator

jwold commented Aug 6, 2020 via email •
edited by westonruter
Loading

@amedina
Copy link
Member

amedina commented Aug 6, 2020

Yeah; much better.

@westonruter
Copy link
Member Author

Also fixing toggle margins on mobile:

Before After
Screen Shot 2020-08-06 at 16 33 58 Screen Shot 2020-08-06 at 16 34 13

@westonruter westonruter mentioned this pull request Aug 6, 2020
Closed
@westonruter
Copy link
Member Author

Added initial fix for cramped buttons on Moto G4:

Before After
image image

For fuller fix, see #5193.

@westonruter
Copy link
Member Author

Removed margin-left from amp-info because when the text fills the horizontal space there can be less margin on the right:

Before After
desktop-before desktop-after
mobile-before mobile-after

@westonruter
Copy link
Member Author

Improved margin for mobile redirection toggle:

Before After
Screen Shot 2020-08-06 at 17 26 59 Screen Shot 2020-08-06 at 17 28 30

@westonruter westonruter requested a review from amedina August 7, 2020 01:24
@amedina
Copy link
Member

amedina commented Aug 7, 2020

All these changes look great! Significant improvements.

One question: the Supported templates section is showing a single part (e.g. content types)?

@westonruter
Copy link
Member Author

One question: the Supported templates section is showing a single part (e.g. content types)?

Ah yes. This is just hidden because I selected Legacy. I should have selected a Reader theme so that supported templates would be shown.

amedina
amedina approved these changes Aug 7, 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.

Significant improvements indeed.

@jwold
Copy link
Collaborator

jwold commented Aug 7, 2020 •
edited
Loading

@westonruter I didn't see anything on desktop that stood out to me. On mobile I found a number of issues that we may want to address:

MOBILE - ONBOARDING

MOBILE - SETTINGS

Granted, some of these are not related to fonts, but if we think they're worth doing I can open a new issue for them.

cc @amedina

@westonruter
Copy link
Member Author

Center align in what way? I think this looks ok to me.

And what should the resolution be? Hide the warning labels on mobile?

@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into fix/font-…
73d0b8d 
…sizes

* 'develop' of github.com:ampproject/amp-wp: (44 commits)
  Mock Reader themes provider
  Delete obsolete deletion of theme_roots site transient
  Use LoadsCoreThemes in remaining tests
  Reuse variable for condition
  Make ReaderThemes::normalize_theme_data() private
  Explicitly ignore legacy theme since not yet pushed to array
  Remove unused asset dependency
  Add test for ReaderThemes::using_fallback_theme
  Add tests for admin notice printers
  Remove outdated hook dependencies
  Remove screenshot from being required for DesktopScreenshot.propTypes
  Enable the "Customize Theme" button after installing the new Reader theme
  Replace astra with neve for testing since astra is suspended
  Remove user capability check from ReaderThemes::using_fallback_theme
  Ensure AMP legacy theme is being used as a fallback
  Only programmmatically select legacy theme when current Reader theme is unavailable
  Restrict showing legacy fallback notice to admins on themes screen and AMP settings screen
  Add link to reader themes drawer
  Ensure paired browsing template_include filter overrides other filters
  Make registration of core theme directory in tests DRY
  ...
@westonruter
Hide welcome illustration on mobile
e542ee3
@westonruter
Copy link
Member Author

Removed welcome illustration on mobile.

Before After
Screen Shot 2020-08-07 at 20 08 51 Screen Shot 2020-08-07 at 20 10 38

@westonruter
Copy link
Member Author

Problem: Those are h3 headings, and if they are reduced to 1rem (16px) then they'd be the same size as the h4 headings:

image

@westonruter
Copy link
Member Author

Fixed supported templates gap.

Before After
supported-templates-gap-before supported-templates-gap-after

@westonruter
Copy link
Member Author

Hid template mode notices on mobile. Fixed line wrapping of Reader theme drawer.

Before After
template-modes-before template-modes-after

@westonruter westonruter mentioned this pull request Aug 8, 2020
Closed
@westonruter
Copy link
Member Author

westonruter commented Aug 8, 2020 •
edited
Loading

I think since these template modes are the only ones shown on the screen, it is OK to be expanded by default. It also ensures the notices are displayed.

@westonruter
Copy link
Member Author

Fixed left/right margins.

Device Before After
Desktop desktop-left-right-margins-before desktop-left-right-margins-after
Mobile mobile-left-right-margins-before mobile-left-right-margins-after

@westonruter westonruter merged commit 957ffec into develop Aug 8, 2020
@westonruter westonruter deleted the fix/font-sizes branch August 8, 2020 04:47
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Aug 8, 2020
@jwold
Copy link
Collaborator

jwold commented Aug 10, 2020

Center align in what way? I think this looks ok to me.

I'm ok with keeping it as is. Was thinking we would move the objects to the middle of the screen on mobile.

And what should the resolution be? Hide the warning labels on mobile?

Agreed, that makes the most sense to me.

Those are h3 headings, and if they are reduced to 1rem (16px) then they'd be the same size as the h4 headings:

If they semantically need to be h3 and h4, then I agree it makes sense to keep them different sizes.

@westonruter westonruter added the WS:UX Work stream for UX/Front-end label Aug 11, 2020
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

@jwold jwold Awaiting requested review from jwold

Assignees

@jwold jwold

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
Development

Successfully merging this pull request may close these issues.
3 participants
@westonruter @jwold @amedina