Nav unification: add flyout menu to sidebar #46046

frontdevde · 2020-09-30T13:24:07Z

Changes proposed in this Pull Request

This PR adds a flyout menu that appears on hover to the Calypso sidebar.

For details and design see issue #45676

Known issues

This is an initial prototype implementation for the flyout. It's meant to give us an initial feel for how the flyout in Calypso.

There are two known issues, that we'll have to address next:

Right now in Calypso the sidebar container is position:fixed and it's content is scrollable. The overflow:auto on the sidebar prevents the flyout from being visible. We're deactivating it here but this has two side effects: a) the sidebar container isn't scrollable anymore and b) when using the site switcher, the overflow is visible when selecting a site. wp-admin gets around the first issue by not having a scrollable sidebar container but instead adjusting the sidebar positioning via JS based on scroll state. The second issue is not present in wp-admin as there's no in-page site switcher.
On nav elements towards the bottom of the page flyouts get cut off. In wp-admin the flyouts are positioned higher via JS if the content doesn’t fit the screen.
Flyout menus with a lot of items also get cut off. This is an issue with the reader in particular. In wp-admin the same can happen if there are too many items in a menu.

Testing instructions

run yarn && yarn start or use the calypso.live link
add ?flags=nav-unification to the URL
interact with the sidebar and confirm all works as expected

matticbot · 2020-09-30T13:24:10Z

Test live: https://calypso.live/?branch=update/nav-unification-sidebar-flyout-v1

matticbot · 2020-09-30T13:29:42Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

cpapazoglou

Thanks @frontdevde for working on this! Generally it works as expected. We just need to address the following issues:

Let's check that hovering on an element in the flyout is in par with wp-admin

Calypso Now	wp-admin

on breakpoints < 782px it should expand / collapse ................. sorry for that.......
we need to take care of touch devices too eg for width 870px

frontdevde · 2020-09-30T15:45:28Z

Let's check that hovering on an element in the flyout is in par with wp-admin

Agreed, it should definitely be the same in Calypso and wp-admin. I removed the bold font weight. As for the color I'm not certain which side we need to adjust though. In the i2 design, the hovered state of the sidebar is white (see comparison below). Based on that maybe we should change wp-admin to be white instead of Calypso to be blue?

@sfougnier Could you please advise us which color to pick here? Thank you!

Design i2	wp-admin

ghost · 2020-09-30T15:54:46Z

Let's check that hovering on an element in the flyout is in par with wp-admin

Agreed, it should definitely be the same in Calypso and wp-admin. I removed the bold font weight. As for the color I'm not certain which side we need to adjust though. In the i2 design, the hovered state of the sidebar is white (see comparison below). Based on that maybe we should change wp-admin to be white instead of Calypso to be blue?

@sfougnier Could you please advise us which color to pick here? Thank you!

Based on the consensus to stick to wp-admin as much as possible, I think we should keep the blue for now. I have updated this in the designs.

frontdevde · 2020-09-30T15:56:02Z

Based on the consensus to stick to wp-admin as much as possible, I think we should keep the blue for now. I have updated this in the designs.

OK, perfect! Will change it to the blue used in wp-admin right now. Thank you!

cpapazoglou · 2020-10-01T07:48:13Z

I can verify that

Let's check that hovering on an element in the flyout is in par with wp-admin

is now showing the desired effects (colors , bg color, font-weight)

getdave · 2020-10-01T08:20:37Z

Just merged this PR which will no doubt effect this one

#46050 (review)

getdave

Loving the top level flyouts!

I'd say overall we still have UX issues to resolve. I would usually say let's merge and iterate but some regressions have occurred following your rebase.

Issues

Possible to have multiple items in "selected" visual state

This has been reintroduced since my PR #46050 fixed this.

Cannot toggle menu sections open/closed

We can't toggle the sections open/closed. I think this is due to the top-level items now having a dual function of:

Navigating to a route.
Toggling the menu section.

Clicking some sections appears to close other sections

This seems fairly random but watching the capture below you can see clicking on some sections closes another section whereas for others it doesn't.

frontdevde · 2020-10-01T10:59:54Z

Possible to have multiple items in "selected" visual state

Definitely, fixed that just now:

Unrelated to the changes here: I have to say it feels a bit weird that a section can be open but not selected. AFAIR we did that so the menu doesn't jump when opening a different section (which less of an issue in wp-admin because of the full reload). That said, maybe we should consider closing sections when clicking a different section. IIRC @getdave suggested that previously.

Cannot toggle menu sections open/closed

Clicking some sections appears to close other sections

I definitely agree those should be addressed! That said, they seem to be unrelated to this PR?

getdave · 2020-10-01T11:33:34Z

Definitely, fixed that just now:

That's much better now @frontdevde - thanks for fixing that.

Unrelated to the changes here: I have to say it feels a bit weird that a section can be open but not selected.

We decided against this because otherwise if you have a section open and then you click another section you get a double whammy "jump" of

Existing section collapsing.
New section opening (or in the case it's not a section then this doesn't happen).

This felt like a worse experience but I'm happy to be told I'm wrong. Either way we should address in a followup. Let's raise an Issue.

Cannot toggle menu sections open/closed
Clicking some sections appears to close other sections
I definitely agree those should be addressed! That said, they seem to be unrelated to this PR?

Let's tackle in followups. I raised the following Issues: #46089 #46091

getdave

Seeing some strange hover styles in Reader.

If that's unrelated to this PR, let's make an Issue and then I'd be happy to 👍 this PR.

frontdevde · 2020-10-01T12:42:15Z

Seeing some strange hover styles in Reader.
If that's unrelated to this PR, let's make an Issue and then I'd be happy to 👍 this PR.

It's not related to this PR but I adjusted the hover color anyway. Reader has a different markup and css class structure and will need additional attention. We should do this in a seperate PR, we're already doing more than we should in this one imo.

getdave

Thanks for being so diligent on this and putting up with having to rebase in my PR and all my changes.

On last test this look much better than it is in master and so I say merge and followup.

wp-desktop

WordPress Desktop CI Failure for job "wp-desktop-mac".

@frontdevde please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

frontdevde · 2020-10-01T13:21:14Z

^ Just noting that tests passed prior to deploying.
Canary tests for your Calypso changes (62aff133) completed successfully!

frontdevde added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Calypso & wp-admin Navigation All navigation in Calypso and wp-admin, and the unified transitions between the two. labels Sep 30, 2020

frontdevde requested a review from cpapazoglou September 30, 2020 13:24

frontdevde self-assigned this Sep 30, 2020

cpapazoglou requested changes Sep 30, 2020

View reviewed changes

frontdevde added 3 commits October 1, 2020 11:56

Nav unification: add flyout menu to sidebar

424789e

Remove bold font weight on hovered item

842245e

Show flyout only on screen sizes >782px

aa40ac9

frontdevde force-pushed the update/nav-unification-sidebar-flyout-v1 branch from 11008ce to aa40ac9 Compare October 1, 2020 10:00

frontdevde and others added 3 commits October 1, 2020 12:01

Merge branch 'master' into update/nav-unification-sidebar-flyout-v1

acc1b62

Remove var added in merge conflict

3d3fad1

Selected state overrides hover state

99eeb4e

getdave reviewed Oct 1, 2020

View reviewed changes

Ensure blue background only appears in selected state

b61afc2

cpapazoglou mentioned this pull request Oct 1, 2020

Add collapse sidebar support #46045

Merged

3 tasks

getdave reviewed Oct 1, 2020

View reviewed changes

Adjust hover text color in reader

d121630

getdave approved these changes Oct 1, 2020

View reviewed changes

frontdevde merged commit 62aff13 into master Oct 1, 2020

frontdevde deleted the update/nav-unification-sidebar-flyout-v1 branch October 1, 2020 12:46

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 1, 2020

wp-desktop requested changes Oct 1, 2020

View reviewed changes

frontdevde mentioned this pull request Oct 1, 2020

Nav unification - (Re)position flyout menus based on available screen space #46094

Closed

frontdevde mentioned this pull request Oct 21, 2020

Nav unification - allow sidebar to be scrolled (while maintaining flyout menus) #46643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nav unification: add flyout menu to sidebar #46046

Nav unification: add flyout menu to sidebar #46046

frontdevde commented Sep 30, 2020 •

edited

Loading

matticbot commented Sep 30, 2020

matticbot commented Sep 30, 2020

cpapazoglou left a comment •

edited by frontdevde

Loading

frontdevde commented Sep 30, 2020

ghost commented Sep 30, 2020

frontdevde commented Sep 30, 2020

cpapazoglou commented Oct 1, 2020 •

edited

Loading

getdave commented Oct 1, 2020

getdave left a comment

frontdevde commented Oct 1, 2020 •

edited

Loading

getdave commented Oct 1, 2020 •

edited

Loading

getdave left a comment

frontdevde commented Oct 1, 2020

getdave left a comment

wp-desktop left a comment

frontdevde commented Oct 1, 2020

Nav unification: add flyout menu to sidebar #46046

Nav unification: add flyout menu to sidebar #46046

Conversation

frontdevde commented Sep 30, 2020 • edited Loading

Changes proposed in this Pull Request

Known issues

Testing instructions

matticbot commented Sep 30, 2020

matticbot commented Sep 30, 2020

cpapazoglou left a comment • edited by frontdevde Loading

Choose a reason for hiding this comment

frontdevde commented Sep 30, 2020

ghost commented Sep 30, 2020

frontdevde commented Sep 30, 2020

cpapazoglou commented Oct 1, 2020 • edited Loading

getdave commented Oct 1, 2020

getdave left a comment

Choose a reason for hiding this comment

Issues

Possible to have multiple items in "selected" visual state

Cannot toggle menu sections open/closed

Clicking some sections appears to close other sections

frontdevde commented Oct 1, 2020 • edited Loading

getdave commented Oct 1, 2020 • edited Loading

getdave left a comment

Choose a reason for hiding this comment

frontdevde commented Oct 1, 2020

getdave left a comment

Choose a reason for hiding this comment

wp-desktop left a comment

Choose a reason for hiding this comment

frontdevde commented Oct 1, 2020

frontdevde commented Sep 30, 2020 •

edited

Loading

cpapazoglou left a comment •

edited by frontdevde

Loading

cpapazoglou commented Oct 1, 2020 •

edited

Loading

frontdevde commented Oct 1, 2020 •

edited

Loading

getdave commented Oct 1, 2020 •

edited

Loading