Let the menu grow #8756

jasmussen · 2018-08-09T07:41:34Z

This PR lets the menu grow to accommodate long text. Already now in Spanish, the "Code Editor" wraps having to fit both the spanish label and keyboard shortcut. As we add more keyboard shortcuts as well as hints, we need to let the menu grow to accommodate that, up to the mobile breakpoint in width.

This PR accomplishes that using auto widths, nowrap whitespace, and a little Nacin' Spacin' thrown in for code cleanup while I was there.

Screenshots:

Affects #7433

To test, verify that there are no regressions with any menus/popover content, including the switcher and the library.

gziolo · 2018-08-09T07:46:36Z

We introduce stylelint which enforces WordPress standards for SCSS files. You can run it with npm run lint-css to see all issues and npm run lint-css:fix to let the tool fix the following issues:

[0] [2] packages/components/src/popover/style.scss
[0] [2] 150:26 ✖ Unexpected whitespace after "(" selector-pseudo-class-parentheses-space-inside
[0] [2] 150:37 ✖ Unexpected whitespace before ")" selector-pseudo-class-parentheses-space-inside
[0] [2] 155:38 ✖ Unexpected whitespace at end of line no-eol-whitespace
[0] [2] 161:26 ✖ Unexpected whitespace after "(" selector-pseudo-class-parentheses-space-inside
[0] [2] 161:37 ✖ Unexpected whitespace before ")" selector-pseudo-class-parentheses-space-inside
[0] [2] 165:26 ✖ Unexpected whitespace after "(" selector-pseudo-class-parentheses-space-inside
[0] [2] 165:37 ✖ Unexpected whitespace before ")" selector-pseudo-class-parentheses-space-inside
[0] [2] 170:26 ✖ Unexpected whitespace after "(" selector-pseudo-class-parentheses-space-inside
[0] [2] 170:37 ✖ Unexpected whitespace before ")" selector-pseudo-class-parentheses-space-inside
[0] [2] 176:26 ✖ Unexpected whitespace after "(" selector-pseudo-class-parentheses-space-inside
[0] [2] 176:37 ✖ Unexpected whitespace before ")" selector-pseudo-class-parentheses-space-inside
[0] [2] 176:44 ✖ Unexpected whitespace after "(" selector-pseudo-class-parentheses-space-inside
[0] [2] 176:55 ✖ Unexpected whitespace before ")" selector-pseudo-class-parentheses-space-inside
[0] [2] 187:26 ✖ Unexpected whitespace after "(" selector-pseudo-class-parentheses-space-inside
[0] [2] 187:37 ✖ Unexpected whitespace before ")" selector-pseudo-class-parentheses-space-inside
[0] [2] 187:44 ✖ Unexpected whitespace after "(" selector-pseudo-class-parentheses-space-inside
[0] [2] 187:55 ✖ Unexpected whitespace before ")" selector-pseudo-class-parentheses-space-inside

jasmussen · 2018-08-09T07:47:43Z

ORLY, and that linter doesn't allow Nacin-Spacin'? Who designed this thing?

:D

I'll fix things up. But seriously though — space after parenthesis is WordPress coding style no?

noisysocks · 2018-08-09T07:50:40Z

But seriously though — space after parenthesis is WordPress coding style no?

https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#values

Do not pad parentheses with spaces

Weird, huh?

gziolo · 2018-08-09T07:51:08Z

I'll fix things up. But seriously though — space after parenthesis is WordPress coding style no?

I have never studied them, however, @ntwb should know why it works as it works 😃

jasmussen · 2018-08-09T07:52:18Z

Wait what when did this happen?

Also CC: @pento I'M ESCALATING THIS ALL THE WAY 😂

Are we also writing "Wordpress" now?

pento · 2018-08-09T07:53:34Z

WHO WROTE THESE CSS CODING STANDARDS

😡😡😡😡😡😡😡😡😡😡😡😡😡😡😡😡😡😡😡😡😡😡😡😡😡

pento · 2018-08-09T07:57:38Z

Are we also writing "Wordpress" now?

We definitely capital_P_dangit() wherever we find it. 😉

ntwb · 2018-08-09T08:09:06Z

jasmussen · 2018-08-09T08:12:04Z

alert( 'This is not fine!` );

tofumatt · 2018-08-09T09:57:22Z

As someone who advocates for coding standards that vary by language, I like the lack of spaces in our (S)CSS! 😄

tofumatt

What happens if the menu text is too long, say in a locale with long words?

tofumatt · 2018-08-09T09:57:58Z

packages/components/src/menu-item/style.scss

@@ -42,10 +42,14 @@
 		stroke: $dark-gray-900 !important;
 		fill: $dark-gray-900 !important;
 	}
+
+	// Don't wrap text.


Won't this mean it overflows if it's too long?

tofumatt · 2018-08-09T09:58:41Z

packages/components/src/popover/style.scss

 		height: auto;
 		overflow-y: auto;
+
+		// Let the menu scale to fit items.
+		width: auto;


These are literally out of alphabetical order and I find that makes them so tough to read. Not part of the coding standards though, so I gotta just live with it I guess 🤷‍♂️

So, what will happen if you add a really really really really long sentence, one that is larger than 360px minus the chrome, well then a horizontal scrollbar will appear in the menu. And we need the whitespace in order for the menu to grow.

From a philosophical perspective, though, I think it's okay to set "no wrapping" as a hard limit. I can't think of software where I see menu items wrapping text (* quickly flips through macos file menus to verify*). Added to that, I feel like we could also put some onus on translators to test that things look right.

That's my opinion in any case.

tofumatt · 2018-08-09T09:59:50Z

To be clear: I really like this and am just curious about very long names. After that's addressed I think we should 🚢

sarahmonster · 2018-08-09T10:23:25Z

It might make sense to wrap the text for smaller screens, to avoid horizontal scrolling within the popover:

jasmussen · 2018-08-09T10:28:18Z

Without the whitespace rule, the menu can't grow — the menu grows by having a min width, a max width, and a width set to auto. But auto allows text wrapping, whereas if you turn off text wrapping, the menu itself expands.

If we were to want to address super long titles on mobile, we'd need to explicitly set a width. Which is definitely doable, something like on mobile, the menu is always 100% the width of the screen and text wraps, and beyond the mobile breakpoint we flip back to auto width and nowrap.

However it does feel like we're hyper optimizing for catastrophe before it's even happened. Which... I know that sounds strong and I'm REALLY not trying to dismiss any of this legitimate feedback. I'm just wondering if we should postpone fixing the theoretical issue until it's an actual issue?

sarahmonster · 2018-08-09T11:26:25Z

Can plugins extend this area? Totally agreed that if we have control over the text here, it's unlikely to be an issue. But if plugins can extend the popover menu, we may not have a way of monitoring when it's become an issue, beyond "people start to complain and/or stop using things."

Also, if we're running on the assumption that users don't want to use WordPress via a web browser on their mobile device, this probably doesn't need to be addressed now. That may well be the case—I don't have any data here and I wouldn't be surprised if users would prefer to use the app ... but that could also be an effect of "the app is easier to use on my phone" as well, so 🙃.

jasmussen · 2018-08-09T11:47:38Z

Yes, plugins can indeed extend this area. It's a solid point. I will try and see if I can figure something at least for the mobile breakpoints.

jasmussen · 2018-08-10T08:33:31Z

So, I write some CSS to try and make the menu full width on mobile through classic responsive breakpoint work, but then I noticed a bunch of .is-mobile references in the CSS. I tried replicating those, but as far as I can tell, that functionality is completely broken right now.

The key seems to start here: https://github.com/WordPress/gutenberg/blob/master/packages/components/src/popover/index.js#L255

... but I'm not actually sure when this is supposed to work. Is this a prop we set on a per-component boolean? @tofumatt since you're way more skilled in this than I am, is there any chance you can take a look and see what might be up?

Here's how I imagine it should work:

At the mobile breakpoints, the popover component always behaves in the same way, which is to fill the entire screen (see https://github.com/WordPress/gutenberg/blob/master/packages/components/src/popover/style.scss#L8)
Beyond the mobile breakpoint, the is-mobile class disappears and the popover behaves as usual.

youknowriad · 2018-08-15T11:05:45Z

@jasmussen The is-mobile className is definitely a per-component controlled prop. That's how the inserter works on mobile. you need to pass expandOnMobile prop to the Popover component to trigger this behavior.

I'd be very cautious to style changes there :) it affects the inserter.

tofumatt · 2018-08-15T14:58:00Z

Sorry, I somehow just missed that GitHub ping. 😓

tofumatt · 2018-08-15T14:59:44Z

Like @youknowriad said: at any rate I don't think you want to use that class; I'd think you'd want to use breakpoints.

I'm unclear if this is ready for another review or not, it looks the same as it did before. Feel free to ping me if there are unanswered questions. 😄

youknowriad · 2018-08-15T15:01:47Z

To clarify, this class is only added on small screens if the prop is true. So if the behavior wanted here is to make this popover fullscreen on mobile, this is the right way to do it. Use this prop.

This PR lets the menu grow to accommodate long text. Already now in Spanish, the "Code Editor" wraps having to fit both the spanish label and keyboard shortcut. As we add more keyboard shortcuts as well as hints, we need to let the menu grow to accommodate that, up to the mobile breakpoint in width. This PR accomplishes that using auto widths, nowrap whitespace, and a little Nacin' Spacin' thrown in for code cleanup while I was there.

jasmussen · 2018-08-16T07:49:26Z

Now there's a case for mobile, where we allow wrapping. It's not super pretty and we'll probably encourage shorter titles, but it should work:

tofumatt

Hah, it occurs to me: why do we even show the keyboard shortcuts on mobile? We could get a lot more use out of that space by hiding them, and keyboard shortcuts on mobile aren't super useful.

Still, this is better. Let's 🚢 , but maybe we should hide those shortcuts on mobile too. I'll make a PR for it.

tofumatt · 2018-08-16T12:48:42Z

packages/components/src/menu-item/style.scss

@@ -41,11 +41,19 @@
 		// !important allows icons from plugins to be overriden and given a dark-gray fill
 		fill: $dark-gray-900 !important;
 	}
+
+	// Don't wrap text.


Should this be "Don't wrap text on mobile screens."?

tofumatt · 2018-08-16T12:49:38Z

packages/components/src/menu-item/style.scss

+
+	// Don't wrap text.
+	@include break-mobile() {
+		.components-popover:not(.is-mobile) & {


This is a bit confusing because it's used on mobile breakpoints but when :not(.is-mobile) is true. 😕

EDIT: Nevermind, I just understood how this works and apparently @include break-mobile works unlike how I expect.

The "break-mobile" mixin is the confusing aspect. is-mobile never invokes on desktop responsive viewpoints, but the mixin does as it is just a media query. So in a way this is a responsive fix, not solely a mobile fix. And given the whole behavior of the popover changes when is-mobile is present, I'd rather not interfere here.

I will clarify the comment.

lol, paths crossed! Cheers 👍

jasmussen · 2018-08-16T13:16:06Z

The keyboard shortcuts thought is a solid idea, and worth a separate ticket.

Note that there are some considerations for that — you might have a really thin viewport on the desktop, but still have access to the shortcuts. Would we want to show or hide them in this case?

noisysocks · 2018-08-17T05:07:57Z

It looks like this has accidentally made DotTips appear too narrow on desktop:

jasmussen · 2018-08-17T06:18:53Z

Hmm, that is unfortunate. I'll put together a PR to fix. Here's all the tips, for posterity. Although it's suboptimal, I don't feel like it's so bad we need to make a point release to fix:

This fixes a regression introduced as part of #8756. Basically we removed the min-width so as to let the content itself expand the popover. But alas an empty textfield, like the URL input field, did not expand the popover as it should. We could look at adding the min-width to the URL input field instead, this should expand the popover, but given the fallout this min-width removal has caused already, it's probably good to keep it in place as a baseline.

jasmussen added the [Type] Enhancement A suggestion for improvement. label Aug 9, 2018

jasmussen self-assigned this Aug 9, 2018

jasmussen requested a review from a team August 9, 2018 07:41

tofumatt reviewed Aug 9, 2018

View reviewed changes

noisysocks mentioned this pull request Aug 10, 2018

Add new keyboard shortcuts help modal #8316

Merged

4 tasks

jasmussen force-pushed the try/let-the-menu-grow branch from 3dddebe to 763a1ef Compare August 10, 2018 08:17

jasmussen requested a review from tofumatt August 13, 2018 07:48

jasmussen added this to the 3.6 milestone Aug 15, 2018

jasmussen force-pushed the try/let-the-menu-grow branch from 763a1ef to 89cc26a Compare August 16, 2018 07:40

Add case for mobile

873df39

tofumatt approved these changes Aug 16, 2018

View reviewed changes

Try and clarify comment.

221e444

jasmussen merged commit c3211fa into master Aug 16, 2018

jasmussen deleted the try/let-the-menu-grow branch August 16, 2018 13:25

tofumatt mentioned this pull request Aug 16, 2018

Hide keyboard shortcuts on mobile #9050

Closed

jasmussen mentioned this pull request Aug 17, 2018

Move MoreMenu-specific styling away from Popover CSS #9069

Merged

jasmussen mentioned this pull request Aug 17, 2018

Restore min-width to popover. #9086

Merged

aduth mentioned this pull request Oct 19, 2018

Components: Add info prop support to MenuItem, FeatureToggle #10802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let the menu grow #8756

Let the menu grow #8756

jasmussen commented Aug 9, 2018

gziolo commented Aug 9, 2018

jasmussen commented Aug 9, 2018

noisysocks commented Aug 9, 2018

gziolo commented Aug 9, 2018

jasmussen commented Aug 9, 2018 •

edited

Loading

pento commented Aug 9, 2018

pento commented Aug 9, 2018

ntwb commented Aug 9, 2018

jasmussen commented Aug 9, 2018

tofumatt commented Aug 9, 2018

tofumatt left a comment

tofumatt Aug 9, 2018

tofumatt Aug 9, 2018

jasmussen Aug 9, 2018

tofumatt commented Aug 9, 2018 •

edited

Loading

sarahmonster commented Aug 9, 2018

jasmussen commented Aug 9, 2018

sarahmonster commented Aug 9, 2018

jasmussen commented Aug 9, 2018

jasmussen commented Aug 10, 2018

youknowriad commented Aug 15, 2018 •

edited

Loading

tofumatt commented Aug 15, 2018

tofumatt commented Aug 15, 2018

youknowriad commented Aug 15, 2018

jasmussen commented Aug 16, 2018

tofumatt left a comment

tofumatt Aug 16, 2018

tofumatt Aug 16, 2018 •

edited

Loading

jasmussen Aug 16, 2018

tofumatt Aug 16, 2018

jasmussen commented Aug 16, 2018

noisysocks commented Aug 17, 2018

jasmussen commented Aug 17, 2018

Let the menu grow #8756

Let the menu grow #8756

Conversation

jasmussen commented Aug 9, 2018

gziolo commented Aug 9, 2018

jasmussen commented Aug 9, 2018

noisysocks commented Aug 9, 2018

gziolo commented Aug 9, 2018

jasmussen commented Aug 9, 2018 • edited Loading

pento commented Aug 9, 2018

pento commented Aug 9, 2018

ntwb commented Aug 9, 2018

jasmussen commented Aug 9, 2018

tofumatt commented Aug 9, 2018

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt Aug 9, 2018

Choose a reason for hiding this comment

tofumatt Aug 9, 2018

Choose a reason for hiding this comment

jasmussen Aug 9, 2018

Choose a reason for hiding this comment

tofumatt commented Aug 9, 2018 • edited Loading

sarahmonster commented Aug 9, 2018

jasmussen commented Aug 9, 2018

sarahmonster commented Aug 9, 2018

jasmussen commented Aug 9, 2018

jasmussen commented Aug 10, 2018

youknowriad commented Aug 15, 2018 • edited Loading

tofumatt commented Aug 15, 2018

tofumatt commented Aug 15, 2018

youknowriad commented Aug 15, 2018

jasmussen commented Aug 16, 2018

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt Aug 16, 2018

Choose a reason for hiding this comment

tofumatt Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

jasmussen Aug 16, 2018

Choose a reason for hiding this comment

tofumatt Aug 16, 2018

Choose a reason for hiding this comment

jasmussen commented Aug 16, 2018

noisysocks commented Aug 17, 2018

jasmussen commented Aug 17, 2018

jasmussen commented Aug 9, 2018 •

edited

Loading

tofumatt commented Aug 9, 2018 •

edited

Loading

youknowriad commented Aug 15, 2018 •

edited

Loading

tofumatt Aug 16, 2018 •

edited

Loading