Skip to content
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

feat: persist sidebar open/close state #17559

Merged
merged 6 commits into from
Jun 18, 2024
Merged

Conversation

thisisamir98
Copy link
Contributor

@thisisamir98 thisisamir98 commented Jun 12, 2024

Description

Persist Sidebar Open/Close State

This pull request introduces a feature to persist the open/close state of the sidebar across sessions. The state is saved to localStorage using Zustand's persist middleware. This ensures that the sidebar retains its state even after a page refresh or browser restart.

Changes

  1. Define Sidebar Open Status Constants:

    • Introduced SidebarOpenStatus as an object with OPEN, CLOSED, MANUAL_OPEN, and MANUAL_CLOSED states.
    • Created a type SidebarOpenStatus for better TypeScript support.
  2. Update Zustand Store:

    • Added openStatus to track the sidebar's state.
    • Updated actions to handle setting and toggling the openStatus.
    • Used Zustand's persist middleware to store the openStatus in localStorage.
  3. Update Sidebar Component:

    • Modified the useEffect hook to respect the manually set openStatus.
    • Ensured that the sidebar state is set based on the openStatus and screen size breakpoint.

Screenshots/Screencast (for UI changes)

REC-20240612162615.mp4

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 56.25000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 46.32%. Comparing base (aaa41aa) to head (be0eda2).
Report is 50 commits behind head on new-navigation.

Current head be0eda2 differs from pull request most recent head f83d6e8

Please upload reports for the commit f83d6e8 to get more accurate results.

Additional details and impacted files
@@                Coverage Diff                 @@
##           new-navigation   #17559      +/-   ##
==================================================
- Coverage           46.50%   46.32%   -0.18%     
==================================================
  Files                 768      770       +2     
  Lines               25020    25140     +120     
  Branches             5745     5756      +11     
==================================================
+ Hits                11635    11646      +11     
- Misses              11927    12031     +104     
- Partials             1458     1463       +5     

setIsSidebarOpen(!mdBreakpoint);
}, [mdBreakpoint, setIsSidebarOpen]);
if (sideBarOpenStatus !== SidebarOpenStatus.MANUAL_OPEN && sideBarOpenStatus !== SidebarOpenStatus.MANUAL_CLOSED) {
setSidebarOpenStatus(mdBreakpoint ? SidebarOpenStatus.CLOSED : SidebarOpenStatus.OPEN);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we don't care if status was SidebarOpenStatus.CLOSED or SidebarOpenStatus.OPEN, so if we will use instead of those two one SidebarOpenStatus.AUTO to persist status and true/false for openStatus, we can write here

if (sideBarOpenStatus === SidebarOpenStatus.AUTO) {
      setSidebarOpenStatus(!mdBreakpoint);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep agreed. Actually a useEffect is not even needed here since there are no effects in there.

The entire code should be something like

const mdBreaking = ...
const sideBarOpen = sidebarStatus === AUTO ? mdBreaking : sidebarStatus === OPEN;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in f83d6e8

isOpen: boolean;
setIsOpen: (isOpen: boolean) => void;
toggleIsOpen: () => void;
openStatus: SidebarOpenStatus;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openStatus is never set to MANUAL_... statuses, right? - then I'd set it to boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in f83d6e8

Comment on lines 72 to 77
export const SidebarOpenStatus = {
OPEN: 'OPEN',
CLOSED: 'CLOSED',
MANUAL_OPEN: 'MANUAL_OPEN',
MANUAL_CLOSED: 'MANUAL_CLOSED',
} as const;
Copy link
Contributor

@atomrc atomrc Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @svitovyda suggested, this can be simplified to

{
  OPEN, // When the user has decided to open manually
  CLOSED, // when the user has decided to manually close
  AUTO, // Will automatically adapt to the screen size
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in f83d6e8

Copy link

sonarcloud bot commented Jun 18, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@atomrc atomrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid 😎

Copy link
Contributor

@przemvs przemvs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@thisisamir98 thisisamir98 merged commit 728e82e into new-navigation Jun 18, 2024
10 checks passed
@thisisamir98 thisisamir98 deleted the persist-open-status branch June 18, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants