Sidenav: Change to consumer-defined closing behavior

[shoupeva-ithaka]

The problem
I am frustrated with how the sidenav component tries to control its own state when not knowing the context in which a user will be using it. It gives the option to control its visibility through the slide prop, but does not emit an event for when the close button within the sidenav is clicked. This renders any manual control of state for the sidenav useless since there will always be a case where a user can close the sidenav without the containing app knowing.

The benefits of the sidenav are that it organizes hierarchical menu items in a easy to use and good looking fashion. It gives users the option to separate categories into divided sections. The sidenav for those reasons already offers enough use to the user. I don't believe that trying to control its own state should be a function of the sidenav, but rather be deferred to the user's discretion. Function such as what the PharosSidenavButton provide and how the PharosSidenav automatically shows and hide based on device resolution may save time in specific contexts, but automatically removes control from the user and constrains the component in unnecessary ways.

The solution
Get rid of the PharosSidenavButton component. Eliminate any automatic toggling of sidenav state. Emit an event for when the close button is clicked. This defers all control to the user (which is really easy, it takes no time to create a prop and event handler) and prevents any limitation in use.

Alternatives considered
At a minimum, add an event for when the close button is clicked.

Additional information
I think it may be worthwhile to explain my specific use case where i ran into this issue...

While implementing the mobile redesign of the site navigation, I needed to use the sidenav as the new means of navigation. The site navigation would be collapsed into a hamburger button (PharosSidenavButton) and when pressed, would toggle the menu (PharosSidenav). However, when the sidenav is toggled I also need to toggle a backdrop behind it (to prevent interaction with components behind the sidenav as well as for aesthetics). Because I cannot invoke a callback for when the sidenav close button is clicked, I cannot control both the sidenav and backdrop in tandem. Furthermore, adding a callback for the click event on the PharosSidenavButton seems to break its default functionality of toggling the sidenav state.

[chrisjbrown]

I agree with @shoupeva-ithaka's evalutation. these feels like unnecessary constraints on the component that could be managed by the consumer for greater flexibility.

Thinking on what repercussions this change would have. It would be breaking for any consumer that would expect the nav to expand/collapse on it's own.
But I'm not sure where we're using the sidenav component other than the pharos homepage.

[eslawski]

Sounds like a reasonable enhancement. It does feel like this would be a breaking change if we are removing a component and changing behavior. Even if we aren't leveraging it on JSTOR, there might be other external consumers using this package.

[chrisjbrown]

i think it would be a major update
allowing users to update or remain on version 12 until they can accommodate the breaking changes

[david-corneail]

@chrisjbrown I think the Admin site uses the side nav

[shoupeva-ithaka]

I just wanted to add to this, I did already implement the close button event, but did not remove the automatic functionality of the PharosSidenavButton. My change wasn't breaking, but I do agree that removing the button and giving full control to the user would be. I still advocate for that to be implemented at some point.

[brentswisher]

@shoupeva-ithaka - can you look this over and see if there is still work needed for this issue, and if so provide a status update on what is needed, and if not, close the issue? It appears #478 may have handled this but not closed the issue?