fix(sidenav): sidenavmenu stays open when in rail #3626

matthew-chirgwin · 2019-08-01T15:18:56Z

The SideNavMenu component when used in a SideNav (using isRail property) maintains its space for its child items when the rail is collapsed if selected. This change adds a property to determine if the SideNav is open, and in the case of the SideNavMenu, change state if it was open/closed when the rail is open/closed.

Changelog

Changed

Added isSideNavExpanded property to child components of the SideNav
Added gDSFP logic to SideNavMenu to detect and handle this case
Added tests for the above

Testing / Reviewing

Unit tests have been added to check on the gDSFP logic to SideNavMenu to make sure this issue could not return . If more testing is required please let me know and Ill update the PR.

Fixes carbon-design-system#3527

netlify · 2019-08-01T15:27:52Z

Deploy preview for the-carbon-components ready!

Built with commit 94d6a0d

https://deploy-preview-3626--the-carbon-components.netlify.com

netlify · 2019-08-01T15:31:07Z

Deploy preview for carbon-elements ready!

Built with commit 94d6a0d

https://deploy-preview-3626--carbon-elements.netlify.com

netlify · 2019-08-01T15:31:21Z

Deploy preview for carbon-components-react ready!

Built with commit 94d6a0d

https://deploy-preview-3626--carbon-components-react.netlify.com

jnm2377

LTGM! 🎉

tw15egan · 2019-08-01T17:10:40Z

Seems like this PR has broken functionality for other versions of the UI Shell: https://deploy-preview-3626--carbon-components-react.netlify.com/?path=/story/ui-shell--header-base-w-navigation-actions-and-sidenav

I'm not able to open any of the left nav menu's

matthew-chirgwin · 2019-08-01T17:20:53Z

Good spot @tw15egan I will look into that now

tw15egan · 2019-08-01T17:23:23Z

And since this was the main bug blocking the Rail version from being displayed, should we go ahead and add a story for the Rails version in now that it seems to be working properly? Or do we want to wait until keyboard functionality is worked in as well (#3529)?

cc @jnm2377 and @aledavila for your opinions

aledavila · 2019-08-01T17:36:09Z

@tw15egan yea I had suggested we display now if it does work. Since the other one is an enhancement

Fixes issue found with existing behaviour

matthew-chirgwin · 2019-08-02T10:22:12Z

Hi all,

As requested, I have added the story in to the storybook. I don’t know if I need to include the build of the storybook or not in the PR, but have not for now. If it is needed, let me know.

As for the issue @tw15egan found, I have guarded the passing of expansion state so it only does this if isRail is passed. From the testing I have done, this maintains the existing capability, but also fixes this issue.

Just one note on why this happened originally too. I was expecting the expanded prop (if provided) to dictate the rendering of the SideNav (ie it being expanded or not). Even when false, it still renders the navigation section, and its items (this is the case without my change and with it). With the change I have made, theSideNavMenu now only expands/responds if the SideNav has an expanded prop/internal state value of true, hence the change in behaviour. Given the prop description, I’m not sure if the SideNav is doing what it should, or if the prop description need updating.

aledavila · 2019-08-02T16:27:34Z

Hi thanks @matthew-chirgwin

Its looking good so far. But there is still an issue the menu now doesn't pin open on click of the header menu. There are 2 ways that the rail should open. On hover of the side nav or on click of the header menu and it should remain open. I believe it was working before.

tw15egan · 2019-08-02T16:56:31Z

The story looks great, and it seems like you've got open on focus behavior sorted. Awesome! I see that the other stories are now working as intended.

I'm also noticing the issue @aledavila pointed out.

In addition, would it be possible to add the active state to the closed menu item, like so:

Looking good so far, and thanks for all the work you've put into this 👍

Update storybook

matthew-chirgwin · 2019-08-02T19:50:04Z

Hey @aledavila @tw15egan . Thanks for the feedback. I've added the required props/callbacks in to allow the rails story to work as you expected (I had removed them for local testing and did not put them back). That has just been added to the branch now ready for review.

I also had a (quick) look at adding the active state when collapsed. With the change as I have it, an active menu will keep the styling even if I interact with another item:

Is it a must have for this issue, or worth a follow on issue?

aledavila · 2019-08-05T15:09:19Z

Thanks for the update. It seems that now the submenus don't open on hover. But if you click them open and then click the header menu the ones you clicked to open are open. So it seems to be passing in the open prop but it doesn't actually open on hover.

vpicone · 2019-08-05T15:14:48Z

@asudoh @joshblack Is this really the prefered method instead of using context? I don't see how mapping over the children is any more explicit than context, the only difference is it's less flexible and transparent for developers.

asudoh · 2019-08-05T22:57:28Z

Propagate-prop-to-child pattern is one we have employed since the inception of the codebase. It has "unintentional override of props" problem and I personally have considered of switching it to render-props, but I haven't figured out a pattern that works for every affected components (e.g. tabs). That said, using propagate-prop-to-child pattern here is fine to me.

vpicone · 2019-08-06T16:33:50Z

@asudoh a major issue you had with context is that the passing of props wasn't explicit to developers. How is this pattern any different?

asudoh · 2019-08-06T22:17:55Z

Propagate-prop-to-child doesn't break the deterministic behavior of components upon props.

joshblack · 2019-08-06T22:52:57Z

If I understand things correctly, what @asudoh is referring to is that using cloneElement means that these components will work without the provider (let me know if I'm misunderstanding 👍)

In your proposal, Vince, were you suggesting having a default provider embedded in our components or would it noop with the default arguments passed into createContext?

matthew-chirgwin · 2019-08-07T12:52:05Z

Hi all - just as an FYI, I am on vacation this week, which means I won’t be able to make any immediate changes to this PR. I don’t object to updating to use context if that is the preference - I went with props as I started the change from the SideNavMenu, and didn’t realise how many levels of prop passing were required. I can update if needed on Monday. Re your last comment @aledavila , plumbing in the render prop for the header button (as requested) into the SideNav expanded prop means the SideNav become controlled, and thus looses the rails mouse over/out behaviour. Please correct me if I have that wrong. Is there another method/prop you were expecting to enable both?

asudoh

The latest code LGTM 👍 - Thanks @matthew-chirgwin!

Fixes carbon-design-system#3529

matthew-chirgwin · 2019-08-19T08:19:04Z

Hi all, I have also had a look at issue #3529 , and incorparated a fix for that into this PR (see commit 0d177f7 ). If you have any feedback please let me know!

asudoh

The latest update makes sense, just one thing - Thanks @matthew-chirgwin!

asudoh · 2019-08-19T12:47:14Z

packages/react/src/components/UIShell/SideNav.js

-        onMouseEnter={() => handleToggle(true)}
-        onMouseLeave={() => handleToggle(false)}>
-        {children}
+        onMouseEnter={() => handleToggle({}, true)}


Any reason not passing along the event?

Hi @asudoh good point - as it was returning true/false before, it should continue to do so. I will change this to onMouseEnter={() => handleToggle(true, true)} and onMouseLeave={() => handleToggle(false, false)}

as it was returning true/false before, it should continue to do so.

Right. An interesting part is (as you may have noticed) that the old code didn't have one-arg interface. And other callers of handleToggle() passes along the raw event. So I was wondering if we could do:

Suggested change

onMouseEnter={() => handleToggle({}, true)}

onMouseEnter={event => handleToggle(event, true)}

Address review comment

tw15egan

This looks really great! Thanks for making all these changes/improvements👍

designertyler · 2019-08-20T20:39:37Z

From a visual perspective, this looks good. The only thing I wanted to double check is the active state when a menu item is selected. I wasn't able to verify that in the demo and wasn't sure if that's a limitation of the demo or overlooked.

I was hoping to see this state

jnm2377 · 2019-08-20T21:14:39Z

wrt @designertyler comment above. It looks like the active SideNavMenu is just missing isActive={true}.

<SideNavMenu
   renderIcon={Fade16}
   title="Category title"
   isActive={true}>

jnm2377 · 2019-08-20T21:21:07Z

Working as expected when you add the missing prop 👍

add/fix isActive check in menu

matthew-chirgwin · 2019-08-21T10:17:41Z

Hi @designertyler @jnm2377 /all - as it turns out, the was a small bug in the SideNavMenu around adding the active class (which can also be driven by the isActive prop). I have fixed this in my latest commit (so it does actually check it's children/their props), meaning it now has the behaviour you requested, and you don't have to explicitly set the prop on the menu if you set the active state on the child. I also added tests to cover this. I hope that is ok!

designertyler

Looks good to me!

asudoh

Just small things to ensure that it's no lost in the pile of conversation - Thank you for all the updates @matthew-chirgwin!

asudoh · 2019-08-22T06:13:42Z

packages/react/src/components/UIShell/SideNav.js

-        onMouseEnter={() => handleToggle(true)}
-        onMouseLeave={() => handleToggle(false)}>
-        {children}
+        onMouseEnter={() => handleToggle(true, true)}


Suggested change

onMouseEnter={() => handleToggle(true, true)}

onMouseEnter={event => handleToggle(event, true)}

@asudoh - before my changes, handleToggle was called here with just true as the only argument. I can change it to whatever the event is as you suggest, but if someone downstream passes the onToggle property to the SideNav and expects the bool value, would that not be a breaking change for them?

Thank you for putting your thoughts on this and elaborating it @matthew-chirgwin! I now see your point (I missed it, tricked by argument name), and I now think the best is keeping handleToggle() call intact - Meaning, call with one arg.

Hi @asudoh - the only issue with moving to one argument is the effect that would then have on how I changed handleToggle to handle this case (lines 48-50). That change could be pushed into its own function, but would that add any value?

Just leaving a note, though this is merged already; I think the best approach is making handleToggle() obvious that the first arg is optional and and the second arg becomes the first arg when the caller doesn't specify the first arg.

asudoh · 2019-08-22T06:13:57Z

packages/react/src/components/UIShell/SideNav.js

-        onMouseLeave={() => handleToggle(false)}>
-        {children}
+        onMouseEnter={() => handleToggle(true, true)}
+        onMouseLeave={() => handleToggle(false, false)}>


Suggested change

onMouseLeave={() => handleToggle(false, false)}>

onMouseLeave={event => handleToggle(event, false)}>

@asudoh - same here as my comment above

jnm2377 · 2019-08-22T17:49:18Z

Update for reviewers: we decided we'll be going with context based components for UI shell. However, given that we need to update more than just the rails functionality to use context (this is a bigger effort than the scope of this PR), we are merging this in as-is and will update to context in a separate PR.

jnm2377 · 2019-08-22T17:59:15Z

Issue for context: #3797

fix(sidenav): sidenavmenu stays open when in rail

b0866e0

Fixes carbon-design-system#3527

jnm2377 requested review from aledavila and tw15egan August 1, 2019 16:11

jnm2377 approved these changes Aug 1, 2019

View reviewed changes

tw15egan and others added 2 commits August 1, 2019 12:32

Merge branch 'master' into 3527-submenu-not-collapsing-in-rails

b64d688

fix(sidenav): sidenavmenu stays open when in rail

7d8788f

Fixes issue found with existing behaviour

Merge branch 'master' into 3527-submenu-not-collapsing-in-rails

d67f853

fix(sidenav): sidenavmenu stays open when in rail

0a3293a

Update storybook

aledavila added status: ready for review 👀 status: visual review 🔍 status: waiting for author's response 💬 labels Aug 5, 2019

asudoh approved these changes Aug 14, 2019

View reviewed changes

asudoh removed the status: waiting for author's response 💬 label Aug 14, 2019

fix(sidenav): make sidenav open/close via keyboard

0d177f7

Fixes carbon-design-system#3529

matthew-chirgwin force-pushed the 3527-submenu-not-collapsing-in-rails branch from 66e07ff to 0d177f7 Compare August 19, 2019 08:16

asudoh reviewed Aug 19, 2019

View reviewed changes

fix(sidenav): make sidenav open/close via keyboard

0a034e8

Address review comment

matthew-chirgwin force-pushed the 3527-submenu-not-collapsing-in-rails branch from 335d4e8 to 0a034e8 Compare August 19, 2019 14:58

tw15egan approved these changes Aug 20, 2019

View reviewed changes

fix(sidenav): sidenavmenu stays open when in rail

ee67d78

add/fix isActive check in menu

matthew-chirgwin force-pushed the 3527-submenu-not-collapsing-in-rails branch from ea2c5c4 to ee67d78 Compare August 21, 2019 10:14

Merge branch 'master' into 3527-submenu-not-collapsing-in-rails

136970f

designertyler approved these changes Aug 21, 2019

View reviewed changes

Merge branch 'master' into 3527-submenu-not-collapsing-in-rails

340a641

asudoh reviewed Aug 22, 2019

View reviewed changes

Merge branch 'master' into 3527-submenu-not-collapsing-in-rails

94d6a0d

jnm2377 merged commit 92076fd into carbon-design-system:master Aug 22, 2019

jnm2377 mentioned this pull request Aug 22, 2019

[UI Shell] - update to Context #3797

Closed

jnm2377 mentioned this pull request Sep 4, 2019

August 2019 Endgame #3780

Closed

17 tasks

AlanGreene mentioned this pull request Sep 12, 2019

React does not recognize the isSideNavExpanded prop on a DOM element #3970

Closed

2 tasks

jnm2377 mentioned this pull request Sep 16, 2019

feat(ui-shell): add alternative behaviours #3990

Merged

emyarod mentioned this pull request Mar 6, 2020

fix(SideNav): disregard hover on non-rail SideNav when closed #5562

Merged

	onMouseEnter={() => handleToggle({}, true)}
	onMouseEnter={event => handleToggle(event, true)}

	onMouseEnter={() => handleToggle(true, true)}
	onMouseEnter={event => handleToggle(event, true)}

	onMouseLeave={() => handleToggle(false, false)}>
	onMouseLeave={event => handleToggle(event, false)}>

fix(sidenav): sidenavmenu stays open when in rail #3626

fix(sidenav): sidenavmenu stays open when in rail #3626

Conversation

matthew-chirgwin commented Aug 1, 2019

Changelog

Testing / Reviewing

netlify bot commented Aug 1, 2019 • edited Loading

netlify bot commented Aug 1, 2019 • edited Loading

netlify bot commented Aug 1, 2019 • edited Loading

jnm2377 left a comment

Choose a reason for hiding this comment

tw15egan commented Aug 1, 2019

matthew-chirgwin commented Aug 1, 2019

tw15egan commented Aug 1, 2019

aledavila commented Aug 1, 2019

matthew-chirgwin commented Aug 2, 2019

aledavila commented Aug 2, 2019

tw15egan commented Aug 2, 2019

matthew-chirgwin commented Aug 2, 2019

aledavila commented Aug 5, 2019

vpicone commented Aug 5, 2019

asudoh commented Aug 5, 2019 • edited Loading

vpicone commented Aug 6, 2019 • edited Loading

asudoh commented Aug 6, 2019

joshblack commented Aug 6, 2019

matthew-chirgwin commented Aug 7, 2019

asudoh left a comment

Choose a reason for hiding this comment

matthew-chirgwin commented Aug 19, 2019

asudoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudoh Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

tw15egan left a comment

Choose a reason for hiding this comment

designertyler commented Aug 20, 2019

jnm2377 commented Aug 20, 2019

jnm2377 commented Aug 20, 2019

matthew-chirgwin commented Aug 21, 2019

designertyler left a comment

Choose a reason for hiding this comment

asudoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm2377 commented Aug 22, 2019

jnm2377 commented Aug 22, 2019

netlify bot commented Aug 1, 2019 •

edited

Loading

netlify bot commented Aug 1, 2019 •

edited

Loading

netlify bot commented Aug 1, 2019 •

edited

Loading

asudoh commented Aug 5, 2019 •

edited

Loading

vpicone commented Aug 6, 2019 •

edited

Loading

asudoh Aug 20, 2019 •

edited

Loading