-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chrome: Persist the "opened/closed" state of the sidebar panels in localStorage #2533
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2533 +/- ##
==========================================
+ Coverage 31.07% 31.09% +0.01%
==========================================
Files 174 174
Lines 5258 5278 +20
Branches 898 905 +7
==========================================
+ Hits 1634 1641 +7
- Misses 3078 3088 +10
- Partials 546 549 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran through some self usability tests and all seemed to pass. Not giving a code review but experience and design wise this is approved.
59b22aa
to
92d357e
Compare
@@ -441,13 +441,21 @@ export function mode( state = 'visual', action ) { | |||
return state; | |||
} | |||
|
|||
export function preferences( state = { isSidebarOpened: ! isMobile }, action ) { | |||
export function preferences( state = { isSidebarOpened: ! isMobile, panels: { 'post-status': true } }, action ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is pretty long and we might consider breaking out the default to its own constant variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix the notes in #2568
...state, | ||
panels: { | ||
...state.panels, | ||
[ action.panel ]: ! get( state, [ 'panels', action.panel ], false ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any need for get
here? state.panels
seems it should always be an object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed for users who already have the preferences persisted.
*/ | ||
export function isEditorSidebarPanelOpened( state, panel ) { | ||
const panels = getPreference( state, 'panels' ); | ||
return panels ? !! panels[ panel ] : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, not sure we need to test presence of panels
.
@@ -67,17 +74,17 @@ const applyConnect = connect( | |||
( state ) => { | |||
return { | |||
featuredImageId: getEditedPostAttribute( state, 'featured_media' ), | |||
isOpened: isEditorSidebarPanelOpened( state, PANEL_NAME ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should bind props using PANEL_NAME
not only in mapStateToProps
, but also in mapDispatchToProps
, so toggle callback in the visual component doesn't need to know/pass the value.
const icon = `arrow-${ opened ? 'down' : 'right' }`; | ||
const className = classnames( 'components-panel__body', { 'is-opened': opened } ); | ||
const { title, children, opened } = this.props; | ||
const isOpened = opened === undefined ? this.state.opened : opened; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any places this component is used with the state variant now? Wonder if we should just treat it as a controlled component, rather than try to juggle both state and prop values of opened
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not right now, no strong preference.
This PR uses the preferences persisted reducer to persist the state of the sidebar panels.
I needed to add the possibility to make the PanelBody opened flag "controlled".
solves #450 partially