fix(sidebar): Maintain toggle state across file navigation #731
Conversation
|
Verified that @ConradJChan has signed the CLA. Thanks for the pull request! |
| const { fileId: prevFileId, isLarge: prevIsLarge }: Props = prevProps; | ||
| const { view }: State = this.state; | ||
|
|
||
| if (fileId !== prevFileId) { |
There was a problem hiding this comment.
Any reason why you're removing the variables that were here before?
There was a problem hiding this comment.
Mainly that it's a different function, I think. Do you see any issues with it?
There was a problem hiding this comment.
nope, just a difference in style. Fine by me
| }); | ||
|
|
||
| // Clone initial state to allow for state reset on new files | ||
| this.state = { ...this.initialState }; | ||
| this.state = { isLoading: true, isOpen: !!isLarge }; |
There was a problem hiding this comment.
This is fine to set since this component won't unmount while navigating, and we're ok with a reset in behavior on preview close?
There was a problem hiding this comment.
Just checked with the T3 behavior and it is different from what we have in EUA, we should probably create a ticket to address this in elements and EUA. We'll probably need to expose a defaultOpen prop and some sort of toggle callback prop so that EUA can keep track of the state across preview instances
There was a problem hiding this comment.
If you toggle close the sidebar, and then 'X' out of preview, when you preview another file, the sidebar defaults to open always in EUA, but in T3 it remembers what state it was last
There was a problem hiding this comment.
Worst case we just store it to session storage
There was a problem hiding this comment.
Or in the in-memory cache.
| @@ -262,28 +237,11 @@ class ContentSidebar extends PureComponent<Props, State> { | |||
| return defaultView; | |||
There was a problem hiding this comment.
What will happen if the default view is set to something like skills but there are no skills on the file? We can address that in a later change.
There was a problem hiding this comment.
You can get rid of the defaultView concept completely. And remove it from props and this if block. It's a feature that no one really asked for or uses at the moment (it was never documented).
There was a problem hiding this comment.
oh right, sidebar is not GA'ed nor documented, so I'll remove it then
|
@ConradJChan for UX testing, I listed the use cases here in the PR message |
|
Thanks @priyajeet, I've run through the use cases and it seems to be in order |
|
Can you update PR message and description |
ConradJChan
changed the title
|
🎉 This PR is included in version 8.9.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Introduce
isOpenstate in sidebar in order to persist toggle state when navigating across different files