cerner / terra-framework

Terra framework houses composed and higher order react components to help developers quickly build out new applications.
http://terra-ui.com
Apache License 2.0
66 stars 72 forks source link

[Terra Application Navigation] Add compact drawer open/close functionality #954

Closed lawsonjames closed 4 years ago

lawsonjames commented 5 years ago

Feature Request

Description

I am currently consuming the Application Navigation component and there are scenarios where I need to know if the compact drawer is currently open or not and also need the ability to close it if need be.

In the mobile version of my application, if a user opens up the drawer and backgrounds the app, when the app is brought back into the foreground the drawer will still be open. This is problematic in the event that a user receives a notification and selects that notification. When this happens, my application needs to navigate through the app and get the user to a specific view. Although the navigational changes are correctly made, they are all done in the "background" because the drawer is still open upon foregrounding.

Additional Context / Screenshots

@ Mentions

@ryanthemanuel @tbiethman @dkasper-was-taken

tbiethman commented 4 years ago

@lawsonjames In the short term, I would recommend using a key to remount the ApplicationNavigation component in this scenario. This will cause the menu visibility state to be reset.

Internally, we will be discussing how we expect these kinds of workflows to work with ApplicationNavigation. While there are certainly changes we could make to expose more control over that menu, we need to determine if we should make them. I'll post back here when we have a concrete go-forward strategy in place.

lawsonjames commented 4 years ago

@ryanthemanuel @tbiethman @mjhenkes @PaulSolDev Could someone please explain why this issue was closed?

mjhenkes commented 4 years ago

@lawsonjames Sorry, it looks like this is my fault. I logged this issue (https://github.com/cerner/terra-framework/issues/960) to replace this. It was my understanding that this request and issue #960 were to solve the same root issue. Your application sends read 'receipts' when the user views a message and did not want to send those if the menu was presented at small form factors because the user would not actually be viewing the message. With #960 we added the ability for you to know in what state the menu was so that you could prevent sending the receipt until the user had dismissed the menu.

When building application navigation we originally intended the disclosure of the drawer to be driven by the user, not programmatically opened/closed. We should circle back with UX on that assumption @neilpfeiffer

As @tbiethman mentioned above, if you need to reset the state of navigation to 'close' the menu by using a key to remount the react component or refresh the page.

lawsonjames commented 4 years ago

@mjhenkes Forcing a component to unmount/remount via a key is being used as an interim solution, but it is my opinion that this should not be considered a permanent solution to this issue. Unmounting/remounting carries its own issues that have to be accounted for (e.g. losing local state) and can cause complexity for the consuming application.

mjhenkes commented 4 years ago

Re-opening this issue to have further discussion.

neilpfeiffer commented 4 years ago

Thanks for your request @lawsonjames. Having reviewed your request, and understanding the nature for the ask - we are not comfortable establishing a pattern to add this level of exposure of the internal mechanics of the drawer internal-nav component, or any of the state & mechanics of any other application-navigation UI element.

While your immediate concern has largely been focused on only this single UI element, that when in a Mobile/Small viewport, the app page+content is hidden from the user by the navigation drawer being open -- our investigation revealed that this same situation can happen by a variety of application-navigation internal components -- including any notification-dialog (alerts / dirty-data warning / time-out warning), application modals (help / about / user settings), application loading overlays, or any UI exposed from the application utility sockets.

This situation is even further exacerbated in assembled+mixed source apps by multiple contributing content teams. Any single page could have content assembled from more than one concept team, where another concept on the page might create a disclosure (modal/slide panel/pop-up) that covers your concept, therefore creating the same scenario in which you are in a state of uncertainty about the visibility and user’s knowledge of a change in “your” UI (i.e. any concerned consumer’s live content region).

Short of having a universal/global disclosure registry util -- that (1)any page content, plus (2)the application, both can message to, indicating any time a portion or all of the base page content may no longer be visible and then clearing when that UI element is removed, … or … a tightly couple application where the state of any UI element can be aware of any other UI element (i.e. a fully baked native application), there simply will be no effective way for your content to ask for, know, and handle each and every instance of each potentially blocking UI element to then effectively report back a “message has been viewed/read” receipt or update the UI unimpeded. And that is not even including a discussion for the topic about allowing the consumer/content to control the opening and closing of the drawer (and therefore manipulate internal / private state of the parent application).

Terra extended an earlier enhancement to track with a callback the ability to notify when the drawer’s state changes (..terra-framework/issues/#960), which was not necessarily a pattern we were comfortable with at that time either. Now having lived with it a while, and more fully understanding risks, exposing the current internal state of any single UI element for on-demand usage would simply be the start of a landslide to add for other UI elements too. Allowing content to chain custom behaviors off of the state of parent or sibling elements, or have the ability to control them, is an approach that in the end, will weaken the stability of composed applications by creating fixed linkages, which we are preferring to not expose, instead opting for reliability.

At the end of the day, for apps built in a Terra fashion, content ideally should just be content, and not have any care nor communication (and therefore dependency) with the type or state of the container it might be put within.

Our recommendation would be in the situation where you need to change what is presented to users in your application (e.g. the drawer is open, a modal is open, user is on a different screen, etc.), to use routing to redirect your application to a new URI that will reload with appropriate content + application state, something that you do have control and the ability to do. And as long a the dirty-data handlers have been correctly implemented by all content teams, if needed, the application will manage the scenario by prompting users prior to automatically wiping everything away and redirecting. For the continuing problem of not knowing the visibility of new messages, if the new/updated URI method is not a solve, investigate alternate methods for the content itself to reliably “know” that a user has seen (or heard) and interacted with a designated piece of content before sending back “message read” statuses that works well within a web app architecture, even if it that exploration takes you into investigating patterns different from more “traditional” or user expected native application behaviors.

ryanthemanuel commented 4 years ago

Closing per @neilpfeiffer's response

lawsonjames commented 4 years ago

I don’t fully agree with the explanation given as to why this feature will not be implemented. In your response you brought up modals and dialogs as being similar to blocking views as the nav drawer, but in response I would say that as an application that consumes these components we have the ability to programmatically close these, whereas we do not have that ability with the nav drawer (hence the feature request). As a consumer of the terra framework there shouldn't have to be work arounds to impact the display of the application. Consumers should have the ability to determine what is being displayed and when while the framework controls how it is displayed. If there are issues with when an application decides to close a menu programmatically (without any user interaction for instance) - that should be managed and decided on by the application owners.



Not all applications that leverage terra will be simple enough to populate some data into pre-built terra components and we want to provide users with the best experience possible. When foregrounding a mobile application via a notification (or another application switch mechanism) the user would expect to "quickly" view the related information. They don't expect to open it to a menu that was left open that they then manually have to close (after already performing the action of opening the application) and they don't expect to have to wait even longer for the application to reload itself.

I am requesting that this issue be re-opened for more consideration.

Tagging @bb3284 and @ryanthemanuel

bb3284 commented 4 years ago

I completely concur with @lawsonjames analysis on this. Not having this ability is causing a lot of framework related issues as we try to "work around" this. Ultimately we need a programmatic way to dismiss the open nav drawer (leaving the underlying components in their previous state). Note that we aren't calling this method in the absence of a user action, simply an action that occurs on the backgrounding of an application.

lawsonjames commented 4 years ago

@neilpfeiffer @ryanthemanuel Any update here?

tbiethman commented 4 years ago

After some more discussion, we have decided to spike on closing the drawer menu automatically during window blur or visibility change events. Doing so will ensure the drawer is not left open after backgrounding the app and prevent the issue at hand here without putting additional API burden on ApplicationNavigation consumers. Consumers who want this functionality will just need to enable it with a boolean prop.

tbiethman commented 4 years ago

The spike for blur/visibility change events handling the menu dismissal did not result in a working solution. Differences across browser implementations resulted in inconsistent behavior.

Instead, a direct event-based approach is going to be taken (see #1255) . This aligns with our future desires for a larger, cross-component solution for the problem of transient content dismissal.