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

Fix: Font size picker max width on mobile #13264

Merged
merged 1 commit into from Feb 1, 2019

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Jan 9, 2019

Description

The font size picker used justify-content: space-between; to render the items on the screen. On small screens the sidebar may be 750px wide and I think the display results on this screen were suboptimal.
This PR adds a max with to make sure the font size picker looks the same on mobile and desktop.

How has this been tested?

I checked that the font size picker renders as shown in the screenshots

Screenshots

Before:
img_6639
screenshot 2019-01-09 at 16 23 46

After:
img_6638
screenshot 2019-01-09 at 16 16 09

@kjellr
Copy link
Contributor

kjellr left a comment

This is much better, thanks for taking care of it!

While you're in there, would you mind removing the 8px of left margin applied to the custom font size field? With the old method, this margin wasn't really noticeable, but with this approach it makes the spacing a little inconsistent:

Currently:
screen shot 2019-02-01 at 12 49 14 pm

Without that 8px margin:
screen shot 2019-02-01 at 12 49 00 pm

After that, this should be good to go. 👍

@jorgefilipecosta jorgefilipecosta force-pushed the fix/font-size-picker-width-on-mobile branch from 3354e60 to 39e3284 Feb 1, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Feb 1, 2019

Hi @kjellr thank you for the feedback, the margin was removed 👍

@kjellr

kjellr approved these changes Feb 1, 2019

Copy link
Contributor

kjellr left a comment

Looks great. Thanks, @jorgefilipecosta!

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Feb 1, 2019

Instead of a max-width, would justify-content: flex-start be better? This would also help if someone registers longer font names that might overflow.

@kjellr

This comment has been minimized.

Copy link
Contributor

kjellr commented Feb 1, 2019

Instead of a max-width, would justify-content: flex-start be better? This would also help if someone registers longer font names that might overflow.

Hmm. That might be cleaner in the end, but we'd also have to provide margins to each of the items in there (they're currently all margin-less, except for the 8px of left margin on the size field that this PR removes). Also, it looks like the font size dropdown actually has an explicit width assigned currently. So longer theme-provided size names will theoretically get cut off right now:

screen shot 2019-02-01 at 2 29 39 pm

Because of that, I don't think switching to flex-start here would make a big difference right now. Ideally that field would be of variable length, and the size/reset fields would knock down onto a second line if needed.

Fixing all those would probably be helpful, and could be handled here if @jorgefilipecosta is up for it. But think it's fine if we tackle them in another PR. Even if we get to that later, this fixes a visual issue in the meantime.

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Feb 1, 2019

@kjellr That's a fair point, I didn't realise we were applying a max-width there. I definitely see some opportunities to improve the entire font size experience so it seems like this is a good start that can be iterated later.

@jorgefilipecosta jorgefilipecosta merged commit 3a1ddab into master Feb 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/font-size-picker-width-on-mobile branch Feb 1, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Feb 1, 2019

Thank you for the discussions and sharing your thoughts around this @chrisvanpatten and @kjellr. I merged this, for now, I will try to iterate and improve the experience taking the points referred.

@youknowriad youknowriad added this to the 5.0 (Gutenberg) milestone Feb 4, 2019

daniloercoli added a commit that referenced this pull request Feb 5, 2019

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/372-use-RichText-on-Title-block

* 'master' of https://github.com/WordPress/gutenberg: (22 commits)
  Make the modal title styling consistent (#13669)
  Disable navigation block for text mode. (#12185)
  Fix: Linting problem in modal example code (#13671)
  Add myself as a code owner to the annotations (#13672)
  Add more reviewers to CODEOWNERS.md file (#13667)
  Plugin: Remove jQuery heartbeat-to-hooks proxying (#13576)
  Workflow: Add repository CODEOWNERS file (#13604)
  Add a mobile minimum size for form fields (#13639)
  Update edit-save documentation  (#13578)
  Alt image setting (#13631)
  Fix: Allow years lower than 1970 in DateTime component. (#13602)
  Using addQueryArgs to generate Manage All Reusable Blocks link (#13653)
  Bump plugin version to 5.0.0-rc.1 (#13652)
  Update lodash to 4.17.10 (#13651)
  Refreshed PR (#9469)
  Set default values of the width and height input fields according to the actual image dimensions (#7687)
  12647 fix css color picker (#12747)
  Remove "we" from messages (#13644)
  Fix: Font size picker max width on mobile (#13264)
  Fix/issue 12501 menu item aria label
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment