feat(hooks): use-theme hook #3169
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: dcb81a4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@jrgarciadev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis update introduces the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (8)
- .changeset/light-needles-behave.md (1 hunks)
- apps/docs/config/routes.json (1 hunks)
- apps/docs/content/docs/customization/dark-mode.mdx (3 hunks)
- packages/hooks/use-theme/README.md (1 hunks)
- packages/hooks/use-theme/tests/use-theme.test.tsx (1 hunks)
- packages/hooks/use-theme/package.json (1 hunks)
- packages/hooks/use-theme/src/index.ts (1 hunks)
- packages/hooks/use-theme/tsconfig.json (1 hunks)
Files skipped from review due to trivial changes (3)
- .changeset/light-needles-behave.md
- packages/hooks/use-theme/package.json
- packages/hooks/use-theme/tsconfig.json
Additional context used
Markdownlint
packages/hooks/use-theme/README.md
32-32: Expected: 0 or 2; Actual: 1
Trailing spaces
Biome
packages/hooks/use-theme/src/index.ts
[error] 25-25: This variable implicitly has the any type.
Variable declarations without type annotation and initialization implicitly have the any type. Declare a type or initialize the variable with some value.
[error] 46-46: This hook does not specify all of its dependencies: setTheme
This dependency is not specified in the hook dependency list.
packages/hooks/use-theme/__tests__/use-theme.test.tsx
[error] 29-29: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 12-12: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
[error] 13-13: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
[error] 25-25: This let declares a variable that is only assigned once.
'store' is never reassigned.
Safe fix: Use const instead.
Additional comments not posted (5)
packages/hooks/use-theme/README.md (2)
7-11: Installation and basic usage instructions are clear and well-documented.Also applies to: 18-19, 27-28, 36-44
48-55: Contribution guidelines and licensing information are appropriately linked and clear.packages/hooks/use-theme/src/index.ts (1)
4-11: Constants and type definitions for themes are well-structured and clear.Also applies to: 14-15
apps/docs/content/docs/customization/dark-mode.mdx (1)
Line range hint
194-248: Documentation for using theuseThemehook is comprehensive and well-structured.apps/docs/config/routes.json (1)
115-116: The route configuration update correctly reflects the documentation changes for dark mode.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/hooks/use-theme/tests/use-theme.test.tsx (1 hunks)
Additional context used
Biome
packages/hooks/use-theme/__tests__/use-theme.test.tsx
[error] 25-25: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 26-26: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
[error] 12-12: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
[error] 13-13: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
Additional comments not posted (4)
packages/hooks/use-theme/__tests__/use-theme.test.tsx (4)
43-48: This test case correctly checks the initialization behavior when no theme is stored.
50-56: This test case correctly handles initialization with a custom theme.
58-67: This test case correctly handles initialization with a theme stored in localStorage.
69-78: This test case correctly tests the functionality of setting a new theme and its effects on localStorage and the DOM.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/hooks/use-theme/src/index.ts (2)
16-16: Consider using PascalCase for type namesFor consistency with TypeScript naming conventions and other type definitions in this file, consider renaming
customThemetoCustomTheme.-export type customTheme = string; +export type CustomTheme = string;This change would make the type naming more consistent throughout the file.
39-39: Update comment for accuracyThe current comment doesn't accurately reflect the code's behavior. Consider updating it to:
- // return light theme if not specified + // if system theme is not applicable, return the defaultThemeThis change more accurately describes the fallback behavior of the initialization logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/hooks/use-theme/src/index.ts (1 hunks)
🔇 Additional comments (1)
packages/hooks/use-theme/src/index.ts (1)
1-85: Overall assessment: Well-implemented useTheme hook with minor improvement opportunitiesThe
useThemehook is well-implemented, providing a robust solution for theme management. It handles various scenarios including system preference, localStorage persistence, and custom themes. The code is generally well-structured and uses React hooks appropriately.A few minor improvements have been suggested:
- Consistent naming convention for types
- More accurate comments
- Optimizing useCallback dependency arrays
- Removing a potentially redundant useEffect
These changes would further enhance the code's clarity, consistency, and efficiency. Great job on implementing this feature!
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/hooks/use-theme/src/index.ts (1 hunks)
🔇 Additional comments (5)
packages/hooks/use-theme/src/index.ts (5)
1-13: LGTM: Imports and constants are well-definedThe imports are correct and necessary for the hook implementation. The
ThemePropsconstant is well-defined with clear, descriptive keys for localStorage and theme values. The use of 'as const' ensures type safety, which is a good practice.
15-21: LGTM: Type definitions are clear and flexibleThe type definitions for
customThemeandThemeare well-structured. TheThemetype, being a union of predefined themes andcustomTheme, allows for both standard and custom theme usage, providing flexibility while maintaining type safety.
29-45: LGTM: Hook initialization and state managementThe
useThemehook is well-implemented with proper initialization and state management. It correctly handles the default theme, stored theme, and system preference.
76-85: LGTM: System preference listener and hook returnThe implementation of the system preference listener and the hook's return value is correct and well-structured.
47-63: 🛠️ Refactor suggestionUpdate dependency array in setTheme useCallback
The
setThemefunction uses theMEDIAconstant, but it's not included in the dependency array. AlthoughMEDIAis unlikely to change, including it would make the code more robust.Consider updating the dependency array:
- [theme], + [theme, MEDIA],Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/docs/content/docs/customization/dark-mode.mdx (1)
218-227: LGTM with minor formatting suggestionThe code example correctly demonstrates the usage of the new
useThemehook from@nextui-org/use-theme. The theme is properly applied to the main element's className.Consider adjusting the indentation of the JSX in the return statement for consistency:
return ( <main className={`${theme} text-foreground bg-background`}> - <App /> + <App /> </main> )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/docs/content/docs/customization/dark-mode.mdx (3 hunks)
🔇 Additional comments (4)
apps/docs/content/docs/customization/dark-mode.mdx (4)
194-197: LGTM: Updated section title and introductionThe changes accurately reflect the transition from
use-dark-modeto@nextui-org/use-theme. The new hook is correctly introduced and linked to its GitHub repository.
201-209: LGTM: Updated installation instructionsThe installation instructions have been correctly updated to use
@nextui-org/use-themeinstead ofuse-dark-mode. The package manager commands are accurate for npm, yarn, and pnpm.
239-250: LGTM: Updated ThemeSwitcher componentThe ThemeSwitcher component has been correctly updated to use the
useThemehook from@nextui-org/use-theme. The theme switching logic now usessetTheme('light')andsetTheme('dark'), which is consistent with the new API.
254-256: LGTM: Retained important note about custom themesThe note about using custom theme names has been correctly retained. This information is still relevant and important for users who want to create and use custom themes with the new
useThemehook.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/hooks/use-theme/src/index.ts (1)
1-21: LGTM! Consider usingas constfor better type inference.The imports, constant definitions, and type declarations are well-structured and provide a solid foundation for the theme management system. The use of a union type for
Themeallows for both predefined and custom themes, which is flexible.For improved type safety, consider using
as constassertion forThemeProps:export const ThemeProps = { KEY: "nextui-theme", LIGHT: "light", DARK: "dark", SYSTEM: "system", } as const;This will ensure that the properties are treated as literal types rather than string types, providing better type inference throughout the codebase.
packages/hooks/use-theme/__tests__/use-theme.test.tsx (2)
27-43: Consider using 'const' for the store objectThe localStorageMock implementation looks good overall. However, you could improve it slightly by using 'const' instead of 'let' for the store object, as it's never reassigned.
- let store: {[key: string]: string} = {}; + const store: {[key: string]: string} = {};This change would make the code more robust by preventing accidental reassignment of the store object.
54-146: LGTM: Comprehensive test coverage with a suggestion for improvementThe test cases provide excellent coverage for the useTheme hook, including initialization, localStorage interaction, and theme changes. The tests are well-structured and focus on specific behaviors.
Consider adding a test case for the theme change listener functionality. This would ensure that the hook properly responds to system theme changes. Here's a suggested structure:
it("should update theme when system preference changes", () => { let mediaQueryList; Object.defineProperty(window, "matchMedia", { writable: true, value: jest.fn().mockImplementation((query) => { mediaQueryList = { matches: false, media: query, onchange: null, addEventListener: jest.fn((_, handler) => handler({ matches: true })), removeEventListener: jest.fn(), dispatchEvent: jest.fn(), }; return mediaQueryList; }), }); const { getByTestId } = render(<TestComponent />); act(() => { getByText("Set System").click(); }); expect(getByTestId("theme-display").textContent).toBe(ThemeProps.SYSTEM); expect(document.documentElement.classList.contains(ThemeProps.LIGHT)).toBe(true); act(() => { mediaQueryList.addEventListener.mock.calls[0][1]({ matches: true }); }); expect(document.documentElement.classList.contains(ThemeProps.DARK)).toBe(true); });This test would simulate a change in the system's color scheme preference and verify that the hook responds correctly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/hooks/use-theme/tests/use-theme.test.tsx (1 hunks)
- packages/hooks/use-theme/src/index.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/hooks/use-theme/src/index.ts (1)
84-85: LGTM! Clear and concise hook return value.The hook's return value is well-structured, providing both the current theme and the function to update it. This follows common React hook patterns and provides a clean API for components to interact with the theme management system.
packages/hooks/use-theme/__tests__/use-theme.test.tsx (3)
1-25: LGTM: Imports and TestComponent are well-structuredThe imports are appropriate for the test file, and the TestComponent is well-implemented using the useTheme hook. The component provides buttons to change themes, which will be useful for testing different scenarios.
45-53: LGTM: Test suite setup is well-structuredThe test suite setup is well-implemented. The use of beforeEach to clear mocks, localStorage, and reset the document class ensures a clean state for each test, which is a good practice.
1-147: Overall: Excellent test file with minor suggestions for improvementThis test file for the useTheme hook is well-structured, comprehensive, and follows good testing practices. It covers various scenarios including initialization, localStorage interaction, and theme changes. The TestComponent and localStorageMock are well-implemented, and the test cases are clear and focused.
The suggestions for improvement (using 'const' in localStorageMock and adding a test for theme change listener) are minor and would further enhance the already strong test suite.
Great job on creating a robust set of tests for the useTheme hook!
* feat(docs): update dark mode content * feat(hooks): @nextui-org/use-theme * chore(docs): revise ThemeSwitcher code * refactor(hooks): simplify useTheme and support custom theme names * feat(hooks): add use-theme test cases * feat(changeset): add changeset * refactor(hooks): make localStorageMock globally and clear before each test * fix(docs): typo * fix(hooks): coderabbitai comments * chore(hooks): remove unnecessary + * chore(changeset): change to minor * feat(hooks): handle system theme * chore(hooks): add EOL * refactor(hooks): add default theme * refactor(hooks): revise useTheme * refactor(hooks): resolve pr comments * refactor(hooks): resolve pr comments * refactor(hooks): resolve pr comments * refactor(hooks): remove unused theme in dependency array * chore(docs): typos * refactor(hooks): mark system as key for system theme * chore: merged with canary --------- Co-authored-by: Junior Garcia <jrgarciadev@gmail.com>
Closes #
📝 Description
use-dark-mode⛳️ Current behavior (updates)
🚀 New behavior
useTheme(ThemeProps.LIGHT):useTheme(ThemeProps.DARK):useTheme(ThemeProps.SYSTEM):💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
Summary by CodeRabbit
New Features
useThemehook for managing and switching between light and dark themes.Documentation
use-dark-modehook to the@nextui-org/use-themehook.