Closed d-e-v-esh closed 2 years ago
Thank you for your contribution @d-e-v-esh , to answer your question
Since the theme variable can only contain two values (light and dark), can we convert that into a boolean? That would make the logic much simpler.
The plan is to allow multiple themes for use to choose from. Currently there are only light and dark themes available but there can be more in future. Therefore, we do not have the value as boolean ATM.
@GMishx Thanks for the clarification. š
@Shruti3004 @sjha2048 can on of you please review this PR and merge it?
I deleted the repo by mistake, I'll create another PR and link it here. š
Refer to #205 for the open PR.
Signed-off-by: Devesh deves125@gmail.com
Description
This bug was occurring because of a few reasons:
useContext
hook was implemented incorrectly. The way it was implemented is a very common mistake so it was easy to catch.Changes
useContext
implementation.My Suggestion
Since the theme variable can only contain two values (
light
anddark
), can we convert that into a boolean? That would make the logic much simpler.In the context file, we are exporting a
state
variable and asetTheme
variable which might cause confusion. I think converting the state to a boolean will solve all the confusion. The code seems too complex for such basic logic.I did not fix the naming convention. I would like to discuss it before changing anything as it could cause confusion. š
Screenshot
How to test
You can simply toggle the theme change button
fixes #165