-
Notifications
You must be signed in to change notification settings - Fork 317
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
fix: remove console log messages on theme change #832
fix: remove console log messages on theme change #832
Conversation
@@ -58,10 +58,6 @@ const App = () => { | |||
localStorage.setItem("dark", !theme); | |||
} | |||
|
|||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If u remove the useEffect here, how will the component re-render after the theme change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kajol-Kumari I think in this case it doesn't perform any side effects or log messages based on theme changes, and if the component doesn't rely on these changes to trigger a re-render, we can safely remove the useEffect block. The component will still re-render when its state or props change. What are your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue that this pull request solves
Closes: #826
Proposed changes
This pull request addresses the issue of console log messages appearing when the theme is changed. The presence of these log messages is undesirable and can affect the user experience. The proposed changes aim to fix or remove the console log statements related to theme changes.
Changes:
Testing:
Types of changes
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply@thevirengarg please review this PR.
Thanks