Skip to content

Conversation

@anmolsinghbhatia
Copy link
Collaborator

@anmolsinghbhatia anmolsinghbhatia commented Jan 8, 2025

Description

This PR updates app theme store.

Type of Change

  • Code refactoring

References

[WEB-2943]

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
web/core/store/theme.store.ts (2)

110-117: Add JSDoc comments for consistency.

The new toggle methods are missing JSDoc comments, unlike other methods in the class.

Add documentation following the existing pattern:

+  /**
+   * Toggle the initiatives sidebar collapsed state
+   * @param collapsed
+   */
   toggleInitiativesSidebar = (collapsed?: boolean) => {

+  /**
+   * Toggle the project overview sidebar collapsed state
+   * @param collapsed
+   */
   toggleProjectOverviewSidebar = (collapsed?: boolean) => {

Also applies to: 119-126


110-126: Consider implementing localStorage cleanup.

All toggle methods store values in localStorage but there's no cleanup mechanism. Consider implementing a cleanup method to remove these values when they're no longer needed.

Example implementation:

cleanup() {
  localStorage.removeItem("initiatives_sidebar_collapsed");
  localStorage.removeItem("project_overview_sidebar_collapsed");
  // ... cleanup other items
}

This method could be called when the store is disposed or when the user logs out.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ce4a375 and ee2f8b2.

📒 Files selected for processing (1)
  • web/core/store/theme.store.ts (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: lint-web
  • GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
web/core/store/theme.store.ts (4)

10-11: LGTM! Interface additions follow established patterns.

The new interface members are well-structured and consistent with the existing codebase patterns.

Also applies to: 18-19


29-30: LGTM! Property declarations are consistent.

The new properties are correctly initialized and follow the established pattern.


40-41: LGTM! MobX decorators are properly configured.

The observable and action decorators are correctly applied, maintaining consistency with the store's reactive pattern.

Also applies to: 48-49


10-11: Verify sidebar state usage in components.

Let's verify that the new sidebar states are properly integrated with the components that will use them.

Comment on lines +29 to +30
initiativesSidebarCollapsed: boolean | undefined = undefined;
projectOverviewSidebarCollapsed: boolean | undefined = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Initialize sidebar states from localStorage.

The new sidebar states should be initialized from localStorage in the constructor, similar to other sidebar states.

Add initialization in the constructor:

   constructor() {
+    // Initialize from localStorage
+    this.initiativesSidebarCollapsed = localStorage.getItem("initiatives_sidebar_collapsed") === "true";
+    this.projectOverviewSidebarCollapsed = localStorage.getItem("project_overview_sidebar_collapsed") === "true";

     makeObservable(this, {

Committable suggestion skipped: line range outside the PR's diff.

@makeplane makeplane deleted a comment from coderabbitai bot Jan 8, 2025
@pushya22 pushya22 merged commit e3ceb48 into preview Jan 10, 2025
13 of 14 checks passed
@pushya22 pushya22 deleted the chore-app-theme-store branch January 10, 2025 04:51
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.

4 participants