toggle button #3814
toggle button #3814
Conversation
Aryankadyan
requested review from
derberg,
akshatnema,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThis update introduces a dark mode feature. A new React component, DarkModeToggle, is added for toggling between light and dark themes, managing theme state via hooks, and updating local storage and the document’s CSS class. The main app component is modified to initialize the theme, conditionally style the UI, and incorporate the toggle button in its layout. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant DMT as DarkModeToggle
participant App as MyApp
participant DOM as Document
participant LS as LocalStorage
U->>DMT: Click toggle button
DMT->>DMT: Execute toggleTheme() to switch theme
DMT->>LS: Update theme value in local storage
DMT->>DOM: Update document’s CSS class based on new theme
App->>DOM: Render UI with theme-dependent styling
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
|
We require all PRs to follow Conventional Commits specification. |
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I have done some changes, for the Dark Mode Toggle Button. If it works in your deployment, please merge my code and if not, please give me feedback. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
pages/_app.tsx (1)
1-16: 🛠️ Refactor suggestionSort imports according to linting rules.
The pipeline is showing an error about import sorting.
import '../styles/globals.css'; import type { AppProps } from 'next/app'; import Head from 'next/head'; import { appWithTranslation } from 'next-i18next'; -import React, { useEffect, useState } from 'react'; +import React from 'react'; import AlgoliaSearch from '@/components/AlgoliaSearch'; +import DarkModeToggle from '@/components/buttons/DarkModeToggle'; import ScrollButton from '@/components/buttons/ScrollButton'; -import DarkModeToggle from '@/components/buttons/DarkModeToggle'; import Banner from '@/components/campaigns/Banner'; import Footer from '@/components/footer/Footer'; import Layout from '@/components/layout/Layout'; import NavBar from '@/components/navigation/NavBar'; import StickyNavbar from '@/components/navigation/StickyNavbar'; import AppContext from '@/context/AppContext';🧰 Tools
🪛 ESLint
[error] 1-16: Run autofix to sort these imports!
(simple-import-sort/imports)
🪛 GitHub Actions: PR testing - if Node project
[error] 1-1: Run autofix to sort these imports! simple-import-sort/imports
🧹 Nitpick comments (3)
components/buttons/DarkModeToggle.tsx (3)
25-25: Fix Tailwind CSS classnames order.The pipeline is showing a warning about invalid Tailwind CSS classnames order.
- className='p-2 rounded-full bg-gray-200 dark:bg-gray-800 text-black dark:text-white focus:outline-none' + className='rounded-full bg-gray-200 p-2 text-black focus:outline-none dark:bg-gray-800 dark:text-white'🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[warning] 25-25: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
1-6: Consider adding server-side rendering support.The current implementation might cause hydration mismatches during SSR because localStorage is only available on the client. Consider using the
useEffecthook to initialize the theme state during client-side rendering.- const [theme, setTheme] = useState<'light' | 'dark'>( - typeof window !== 'undefined' && localStorage.getItem('theme') === 'dark' ? 'dark' : 'light' - ); + const [theme, setTheme] = useState<'light' | 'dark'>('light'); + + useEffect(() => { + const savedTheme = localStorage.getItem('theme'); + if (savedTheme === 'dark') { + setTheme('dark'); + } + }, []);
27-27: Consider using SVG icons for better visual consistency.Using emoji characters for the toggle icons might look different across platforms. Consider using SVG icons for a more consistent appearance.
You could import icons from a library like heroicons:
+ import { SunIcon, MoonIcon } from '@heroicons/react/24/solid'; // ... - {theme === 'light' ? '🌙' : '☀️'} + {theme === 'light' ? <MoonIcon className="h-5 w-5" /> : <SunIcon className="h-5 w-5" />}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/buttons/DarkModeToggle.tsx(1 hunks)pages/_app.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: PR testing - if Node project
components/buttons/DarkModeToggle.tsx
[warning] 25-25: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
pages/_app.tsx
[error] 1-1: Run autofix to sort these imports! simple-import-sort/imports
[warning] 18-18: Missing JSDoc comment. require-jsdoc
[error] 19-19: 'setTheme' is assigned a value but never used. unused-imports/no-unused-vars
[error] 19-19: 'setTheme' is assigned a value but never used. no-unused-vars
[error] 37-37: Strings must use singlequote. quotes
🪛 ESLint
pages/_app.tsx
[error] 19-19: 'setTheme' is assigned a value but never used.
(unused-imports/no-unused-vars)
[error] 19-19: 'setTheme' is assigned a value but never used.
(no-unused-vars)
[error] 37-37: Strings must use singlequote.
(quotes)
⏰ Context from checks skipped due to timeout of 180000ms (4)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Lighthouse CI
🔇 Additional comments (2)
components/buttons/DarkModeToggle.tsx (1)
3-30: Nice implementation of the DarkModeToggle component!The component correctly:
- Initializes the theme state based on localStorage
- Uses useEffect to update the DOM and localStorage
- Provides a clean toggle function
- Renders a button with appropriate styling based on the current theme
🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[warning] 25-25: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
pages/_app.tsx (1)
40-43: Great implementation of the toggle button placement!Good job positioning the dark mode toggle button in the navbar. The flex layout works well for placing it alongside the navigation.
| import Banner from '@/components/campaigns/Banner'; | ||
| import Footer from '@/components/footer/Footer'; | ||
| import Layout from '@/components/layout/Layout'; | ||
| import NavBar from '@/components/navigation/NavBar'; | ||
| import StickyNavbar from '@/components/navigation/StickyNavbar'; | ||
| import AppContext from '@/context/AppContext'; | ||
|
|
||
| /** | ||
| * @description The MyApp component is the root component for the application. | ||
| */ | ||
| function MyApp({ Component, pageProps, router }: AppProps) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc comment to the MyApp function.
The linting rules require JSDoc comments for functions.
+/**
+ * Main application component that wraps all pages
+ * @param {AppProps} props - The app props including Component, pageProps, and router
+ * @returns {JSX.Element} The rendered application
+ */
function MyApp({ Component, pageProps, router }: AppProps) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function MyApp({ Component, pageProps, router }: AppProps) { | |
| /** | |
| * Main application component that wraps all pages | |
| * @param {AppProps} props - The app props including Component, pageProps, and router | |
| * @returns {JSX.Element} The rendered application | |
| */ | |
| function MyApp({ Component, pageProps, router }: AppProps) { |
🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[warning] 18-18: Missing JSDoc comment. require-jsdoc
| @@ -3,32 +3,44 @@ | |||
| import type { AppProps } from 'next/app'; | |||
| import Head from 'next/head'; | |||
| import { appWithTranslation } from 'next-i18next'; | |||
| import React from 'react'; | |||
| import React, { useEffect, useState } from 'react'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused imports after removing duplicate theme logic.
After removing the duplicate theme state management, you should also remove the unused imports.
-import React, { useEffect, useState } from 'react';
+import React from 'react';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import React, { useEffect, useState } from 'react'; | |
| import React from 'react'; |
| <Head> | ||
| <script async defer src='https://buttons.github.io/buttons.js'></script> | ||
| </Head> | ||
| <AlgoliaSearch> | ||
| <div className='flex min-h-screen flex-col'> | ||
| <div className={`flex min-h-screen flex-col bg-white text-black dark:bg-black dark:text-white`}> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix string quote style.
According to your linting rules, strings should use single quotes instead of double quotes.
- <div className={`flex min-h-screen flex-col bg-white text-black dark:bg-black dark:text-white`}>
+ <div className={'flex min-h-screen flex-col bg-white text-black dark:bg-black dark:text-white'}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className={`flex min-h-screen flex-col bg-white text-black dark:bg-black dark:text-white`}> | |
| <div className={'flex min-h-screen flex-col bg-white text-black dark:bg-black dark:text-white'}> |
🧰 Tools
🪛 ESLint
[error] 37-37: Strings must use singlequote.
(quotes)
🪛 GitHub Actions: PR testing - if Node project
[error] 37-37: Strings must use singlequote. quotes
| const [theme, setTheme] = useState<'light' | 'dark'>( | ||
| typeof window !== 'undefined' && localStorage.getItem('theme') === 'dark' ? 'dark' : 'light' | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (theme === 'dark') { | ||
| document.documentElement.classList.add('dark'); | ||
| } else { | ||
| document.documentElement.classList.remove('dark'); | ||
| } | ||
| }, [theme]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate theme state management.
There's duplicate theme state management between _app.tsx and DarkModeToggle.tsx. Since the toggle component already handles theme state and persistence, you should remove this duplicate logic from _app.tsx.
- const [theme, setTheme] = useState<'light' | 'dark'>(
- typeof window !== 'undefined' && localStorage.getItem('theme') === 'dark' ? 'dark' : 'light'
- );
-
- useEffect(() => {
- if (theme === 'dark') {
- document.documentElement.classList.add('dark');
- } else {
- document.documentElement.classList.remove('dark');
- }
- }, [theme]);This will also fix the linting errors related to the unused setTheme variable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [theme, setTheme] = useState<'light' | 'dark'>( | |
| typeof window !== 'undefined' && localStorage.getItem('theme') === 'dark' ? 'dark' : 'light' | |
| ); | |
| useEffect(() => { | |
| if (theme === 'dark') { | |
| document.documentElement.classList.add('dark'); | |
| } else { | |
| document.documentElement.classList.remove('dark'); | |
| } | |
| }, [theme]); |
🧰 Tools
🪛 ESLint
[error] 19-19: 'setTheme' is assigned a value but never used.
(unused-imports/no-unused-vars)
[error] 19-19: 'setTheme' is assigned a value but never used.
(no-unused-vars)
🪛 GitHub Actions: PR testing - if Node project
[error] 19-19: 'setTheme' is assigned a value but never used. unused-imports/no-unused-vars
[error] 19-19: 'setTheme' is assigned a value but never used. no-unused-vars
Description
Implemented Dark Mode Toggle: Added a button to switch between light and dark themes.
Theme Persistence: Stores user preference in localStorage and applies it on page load.
Dynamic Theme Switching: Updates document.documentElement.classList to apply the dark class.
Related issue(s)
Summary by CodeRabbit