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

Improve inserter UI on mobile devices #5514

Merged
merged 2 commits into from Mar 22, 2018

Conversation

Projects
None yet
4 participants
@noisysocks
Member

noisysocks commented Mar 8, 2018

Description

Closes #4077.

Before After
local wordpress test_wp-admin_post-new php_gutenberg-demo iphone 6_7_8 3 local wordpress test_wp-admin_post-new php_gutenberg-demo iphone 6_7_8 2

Tweaks the inserter UI on mobile to display the post title in a header above the search bar. We accomplish this by adding a new headerTitle property to <Popover> and <Dropdown>.

How Has This Been Tested?

  1. Check that the new UI behaves correctly when viewed on a mobile device
  2. Check that long post titles appear truncated with an ellipsis
  3. Check that the UI has not changed when viewed on a desktop
@brandonpayton

This comment has been minimized.

Show comment
Hide comment
@brandonpayton

brandonpayton Mar 8, 2018

Member

The implementation looks good to me, but I think it would be good to find a prop name that describes the purpose of the text better than headerTitle. It isn’t really the popover title but an indication that the popover exists in the context of editing a post. I haven’t thought of a better name yet but wanted to bikeshed share the concern.

Member

brandonpayton commented Mar 8, 2018

The implementation looks good to me, but I think it would be good to find a prop name that describes the purpose of the text better than headerTitle. It isn’t really the popover title but an indication that the popover exists in the context of editing a post. I haven’t thought of a better name yet but wanted to bikeshed share the concern.

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Mar 9, 2018

Member

I can't think of a better name, either. I was trying to choose a name that makes sense when you consider Popover by itself as standalone component.

Maybe we could adjust the existing expandOnMobile prop to accept options?

// Default:
<Popover />
// Expands on mobile with no title:
<Popover expandOnMobile />
// Expands on mobile with a title:
<Popover expandOnMobile={ { enabled: true, title: 'Title to show when expanded' } } />
Member

noisysocks commented Mar 9, 2018

I can't think of a better name, either. I was trying to choose a name that makes sense when you consider Popover by itself as standalone component.

Maybe we could adjust the existing expandOnMobile prop to accept options?

// Default:
<Popover />
// Expands on mobile with no title:
<Popover expandOnMobile />
// Expands on mobile with a title:
<Popover expandOnMobile={ { enabled: true, title: 'Title to show when expanded' } } />
@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Mar 10, 2018

Member

Thanks for going this, so good to see mobile getting better each PR.

One tiny point, could we adjust the alignment so the title and search line up? I know they are different sizes but that slight out of line would be good to fix.

Member

karmatosed commented Mar 10, 2018

Thanks for going this, so good to see mobile getting better each PR.

One tiny point, could we adjust the alignment so the title and search line up? I know they are different sizes but that slight out of line would be good to fix.

noisysocks added some commits Mar 8, 2018

Improve inserter UI on mobile devices
On mobile devices, display the post title in a header above the inserter
popover UI.
Increase padding of inserter search bar
Increase the text field's padding so that its contents align with the
tabs below and the title above.
@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Mar 20, 2018

Member

Thanks @karmatosed! I increased the padding on the search field so that the post title, Search for a block, and Frequent all align:

local wordpress test_wp-admin_post php_post 218 action edit iphone 6_7_8

Member

noisysocks commented Mar 20, 2018

Thanks @karmatosed! I increased the padding on the search field so that the post title, Search for a block, and Frequent all align:

local wordpress test_wp-admin_post php_post 218 action edit iphone 6_7_8

@noisysocks noisysocks requested a review from youknowriad Mar 20, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Mar 21, 2018

Contributor

Noting that the title also shows up when you open the sidebar. Was this intended?

--
I'm no designer but is it obvious that this text is the post title? Also, the fact that it doesn' really look like a header is weird to me, but again I'm no designer just my two cents.

Code wise, this looks good.

Contributor

youknowriad commented Mar 21, 2018

Noting that the title also shows up when you open the sidebar. Was this intended?

--
I'm no designer but is it obvious that this text is the post title? Also, the fact that it doesn' really look like a header is weird to me, but again I'm no designer just my two cents.

Code wise, this looks good.

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Mar 21, 2018

Member

@youknowriad thanks for your feedback. There is a suggestion to make it grey, would you feel that made it more of a header?

@noisysocks thanks the padding looks good.

Member

karmatosed commented Mar 21, 2018

@youknowriad thanks for your feedback. There is a suggestion to make it grey, would you feel that made it more of a header?

@noisysocks thanks the padding looks good.

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Mar 22, 2018

Member

Noting that the title also shows up when you open the sidebar. Was this intended?

Yep, this was done in #5511.

Thanks for reviewing. I'll merge this for now. Let me know if you want to tweak the colours, etc.

Member

noisysocks commented Mar 22, 2018

Noting that the title also shows up when you open the sidebar. Was this intended?

Yep, this was done in #5511.

Thanks for reviewing. I'll merge this for now. Let me know if you want to tweak the colours, etc.

@noisysocks noisysocks merged commit f2518df into master Mar 22, 2018

2 checks passed

codecov/project 44.22% remains the same compared to f4cc3bf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@noisysocks noisysocks deleted the update/mobile-inserter-ui branch Mar 22, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Mar 22, 2018

Contributor

Yes, maybe I'd prefer if it was grey, but happy to try as is for now.

Contributor

youknowriad commented Mar 22, 2018

Yes, maybe I'd prefer if it was grey, but happy to try as is for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment