Disable mobile sidebar from REDUX_REHYDRATE payload. #4283
Conversation
jorgefilipecosta
force-pushed
the
fix/disable-mobile-sidebar
branch
from
c0b7366 to
5284172
Compare
Mobile sidebar should be closed on reload even if it was opened before.
jorgefilipecosta
force-pushed
the
fix/disable-mobile-sidebar
branch
from
5284172 to
20bca52
Compare
jorgefilipecosta
added
[Type] Bug
|
Nothing that in #4248 we want to disable persisting for the "publish" sidebar as well. The only sidebar that needs to be persisted is the "desktop" sidebar. So maybe instead of a blacklist, we should do a whitelist to fix these two errors together? |
|
Good point @youknowriad. It looks like desktop sidebar is a preference in its own right persisted across sessions. But mobile, and publish sidebars are not preference they are just transient information for the UI state. I'm thinking on discarding this logic for mobile, and create a new reducer called transientSiderbars that contain the mobile and publish sidebars while desktop continues to be a preference. In your option would that be a preferable approach? If yes I will close this PR and create a new one with this changes. |
|
@jorgefilipecosta That's a good option, but what about an alternative using a REDUX_SERIALIZE action like suggested by @aduth similar to what we have in Calypso? This action if defined would filter the state and return only the preferences that needs to be saved? |
| payload.isSidebarOpenedMobile ? { ...payload, isSidebarOpenedMobile: false } : payload | ||
| export const disableMobileSidebar = ( payload ) => ( | ||
| get( payload, 'sidebars.mobile' ) ? | ||
| { ...payload, ...{ sidebars: { ...payload.sidebars, mobile: false } } } : |
There was a problem hiding this comment.
Apologies upfront for my incompetence, but is the second spread operator necessary in this case? isn't this the same as { ...payload, sidebars: { ...payload.sidebars, mobile: false } }?
There was a problem hiding this comment.
Hi @chriskmnds, you are totally right 👍 your code is equivalent and simpler.
|
Hi @youknowriad PR #4529 tries a different approach related to your suggestion :) I'm closing this one for now. |
The mobile sidebar should be closed on reload even if it was opened before. This behavior was implemented, but during a refactor to the sidebars a regression made the logic invalid.
How Has This Been Tested?
Resize to mobile resolutions, open the sidebar, reload the page, and see the sidebar is now closed.
Try to open and close sidebar in desktop and mobile see that things still work.
See that on the desktop the sidebar opened state is still persisted when reloading.