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.

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 Nav Menu overflow using flex-wrap approach #18431

Merged
merged 3 commits into from Dec 3, 2019
Merged

Fix Nav Menu overflow using flex-wrap approach #18431

merged 3 commits into from Dec 3, 2019

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented Nov 11, 2019

Closes #18298

This PR handles navigation menu overflow by simply allowing wrapping of Nav Items. Please see the Issue for more context about the decision making process behind this approach.

Screenshot 2019-11-11 at 14 52 46

Testing

  1. Add new Post
  2. Add Nav Block
  3. Switch Editor to "Code Mode"
  4. Copy testing Block (see below) markup
  5. Paste into Editor code-view window.
  6. Switch back to "Visual" mode.
  7. See large Nav with many items.
  8. Test it wraps and interacts as expected.

Here's a handy long nav for testing

<!-- wp:navigation-menu {"backgroundColorCSSClass":null,"textColorCSSClass":null} -->
<!-- wp:navigation-menu-item {"label":"Sample Page","title":"Sample Page","type":"page","url":"http://localhost:8889/sample-page/"} /-->

<!-- wp:navigation-menu-item {"label":"Test Page!","title":"Test Page!","type":"page","url":"http://localhost:8889/test-page/"} /-->

<!-- wp:navigation-menu-item {"label":"Sample Page","title":"Sample Page","url":"http://localhost:8889/sample-page/"} /-->

<!-- wp:navigation-menu-item {"label":"Sapiente repellendus eius quisquam odio corporis consequuntur","title":"Sapiente repellendus eius quisquam odio corporis consequuntur","url":"http://localhost:8889/sapiente-repellendus-eius-quisquam-odio-corporis-consequuntur/"} /-->

<!-- wp:navigation-menu-item {"label":"Saepe omdddnis commodi veniam dolores tempore est","title":"Saepe omnis commodi veniam dolores tempore est","url":"http://localhost:8889/saepe-omnis-commodi-veniam-dolores-tempore-est/"} /-->

<!-- wp:navigation-menu-item {"label":"Test Page!","title":"Test Page!","url":"http://localhost:8889/test-page/"} /-->

<!-- wp:navigation-menu-item {"label":"Qui ducimus mdddd dolestiae deserunt qui","title":"Qui ducimus molestiae deserunt qui","url":"http://localhost:8889/qui-ducimus-molestiae-deserunt-qui/"} /-->

<!-- wp:navigation-menu-item {"label":"Sapidddddente repellendus eius quisquam odio corporis consequuntur","title":"Sapiente repellendus eius quisquam odio corporis consequuntur","url":"http://localhost:8889/sapiente-repellendus-eius-quisquam-odio-corporis-consequuntur/"} /-->
<!-- /wp:navigation-menu -->

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 14, 2019

This is an alternative fix for #18298, CC: @talldan @mtias.

@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 14, 2019

My only concern is that the nav could now easily not look like a Nav but rather a "flow" of text.

@jasmussen jasmussen marked this pull request as ready for review Nov 14, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 14, 2019

My only concern is that the nav could now easily not look like a Nav but rather a "flow" of text.

I think that's a valid concern, and one that could likely be looked at separately by perhaps defaulting to blue underlined links with some style variations, or perhaps making the items more "menu item-like". Much of this will also be up to the theme, in this screenshot I primarily used the vanilla styles. A theme could do a lot to enhance this.

@shaunandrews

This comment has been minimized.

Copy link

shaunandrews commented Nov 14, 2019

I think wrapping, while a fine mvp solution, is going to cause confusion when dealing with multi-level menus. At some point it will be impossible to tell where you are within the structure of the navigation. Perhaps this isn’t a major concern, but I feel it’s an untenable solution.

@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 14, 2019

@jasmussen @shaunandrews I just pushed a "fix" to solve 2 things

  1. I've flexed the nav items to allow them to occupy a reasonably amount of space based on their content width. This means we have a more "nav like" layout effect with each item clearly distinguishable as an "item".
  2. I've taken the Block movers out of the flow to avoid the layout "jump" when the Block is selected. This ensures all items stay on their original line and things don't change place or jump underneath your cursor.

Let me know if you feel this is a good improvement?

See below for before and after. :

Before

Notice how things change position and the layout jumps. Also, it's not clear that the items are nav items. They look like long lines of paragraph text.

Screen Capture on 2019-11-14 at 14-22-27

After

Notice less layout reflow. Also items are clearly distinguishable as individual nav items:

Screen Capture on 2019-11-14 at 14-21-45

@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 14, 2019

I think wrapping, while a fine mvp solution

@shaunandrews That's good because #18298 (comment)

At some point it will be impossible to tell where you are within the structure of the navigation. Perhaps this isn’t a major concern, but I feel it’s an untenable solution.

I think your "mover" proposal will be the best long term solution.

@getdave getdave force-pushed the try/nav-wrap branch from 45feeba to d8ea8a6 Nov 19, 2019
@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 19, 2019

I've changed my mind on the UX for this since this experiment.

I prefer keeping things simple and allowing the items to wrap naturally.

The only change I've kept is to slim down the amount of horizontal space occupied by each Nav item.

Here's how this looks now:

Screen Capture on 2019-11-19 at 15-51-48

Now looking for confirmation we're good to merge this from a design POV @shaunandrews. Also pinging @jasmussen as the original PR author.

@getdave getdave added this to 👀 PRs to review in Navigation block via automation Nov 19, 2019
@getdave getdave self-assigned this Nov 19, 2019
@getdave getdave force-pushed the try/nav-wrap branch from d8ea8a6 to f3ff038 Nov 19, 2019
@getdave getdave changed the title Try: Flex-wrap navigation menu Fix Nav Menu overflow using flex-wrap approach Nov 19, 2019
@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Nov 19, 2019

I like how this looks, when there is a multitude of links it resembles a tag cloud :) I wonder what happens with submenus.

(edit)

It looks like this:

Screenshot 2019-11-19 at 18 13 53

like @shaunandrews mentioned there is a point where you loose where exactly you are, visually.

@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 19, 2019

...there is a point where you loose where exactly you are, visually.

An alternative is to force a reasonable set width for each item, thereby ensuring a columular layout which would make it easier to see the submenus. The width would be arbitary and hardcoded though because if you allow flex-box to auto size then you get a lot of jumping when you select an item (due to movers appearing).

I feel like we need to consider the experience in more detail before more coding is done. I will need design support with this...

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 20, 2019

I prefer keeping things simple and allowing the items to wrap naturally.
The only change I've kept is to slim down the amount of horizontal space occupied by each Nav item.

What other change had you made? Just curious. But I do agree, just keep it simple, we'll revisit in a V2 with an overflow menu anyway. Spacing looks good.

there is a point where you loose where exactly you are, visually.

It's important to keep in mind here what is the theme and what is the menu, and not only that, what styles should the block provide vs. what should the theme provide.

The end result is that what the stock menu provides has to, unfortunately, be pretty basic — just links. Otherwise it just creates headaches for themes.

And right now none of those themes have styled the navigation menu, neither front nor backend. So some of this is a chicken and egg situation. Some of it has to be solved in a more holistic way by improving how themes can opt in or out of "opinionated styles" provided by the block. It's just important we don't do "one-off" design improvements that will effectively end up tecnical debt and themer headaches.

Finally, I think this is an opportunity to revisit the block UI itself, and I have some ideas how to address this. But this also has to happen in a holistic way. (I.e. not here. )

@getdave getdave force-pushed the try/nav-wrap branch from f3ff038 to d3fa9c7 Nov 21, 2019
@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 21, 2019

e2e tests are suddenly failing. I've rebased as a first step to resolving this.

@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Nov 28, 2019

Will be switching back to this soon.

jasmussen and others added 3 commits Nov 11, 2019
This is just example code to illustrate a wrapping effect. I might be missing some fundamentals that makes this PR not viable, so it's more a matter of "discussing in code" than an actual PR.
@getdave getdave force-pushed the try/nav-wrap branch from d3fa9c7 to a5c2d8a Dec 2, 2019
@getdave

This comment has been minimized.

Copy link
Contributor

getdave commented Dec 2, 2019

@jasmussen Looks like we're good to go on this. Might need a final design review though...

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 2, 2019

Looks like we're good to go on this. Might need a final design review though...

I see the following in this branch:

Screenshot 2019-12-02 at 19 54 48

Here's what's shipping in the plugin:

Screenshot 2019-12-02 at 19 57 45

I think this is a vast improvement, and thoroughly shippable.

Note: I can't get the menu to work in master though, I'm getting this:

Screenshot 2019-12-02 at 19 57 51

That failure is unrelated to your PR, though, and is possibly an issue on my end. But might want to test master on your end to see if it's broken there for you too, just before you merge. That will make any regressions easier to debug.

Oh, and I can't approve my own PR, but as a design sanity check, 👍 👍

@getdave
getdave approved these changes Dec 3, 2019
Copy link
Contributor

getdave left a comment

Following on from @jasmussen's design review 👍 I'm going to approve this one on his behalf.

@getdave getdave merged commit 7e6c932 into master Dec 3, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Dec 3, 2019
@getdave getdave deleted the try/nav-wrap branch Dec 3, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.