-
Notifications
You must be signed in to change notification settings - Fork 962
Feature/auto light theme #1547
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
Feature/auto light theme #1547
Conversation
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.
Hey @arkthur - thanks for the PR! I had a couple small points of feedback for you, but other than that I see things working quite well.
Screen.Recording.2025-03-10.at.7.45.34.PM.mov
Now I added a custom hook so the listener can be properly removed on unmount and the logic is less repetitive, avoiding redundancy. |
Just pushed a fix for the unit tests, which includes some needed changes to the hook |
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.
Looks good to me! Thanks for making those changes. I'll grab someone else from IR to take a look as well and then hopefully merge.
Thanks!
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.
This all looks good to me!
One last check though, @arkthur could you include a screenshot of the <TreeView />
since there were changes there. Or a video of the timeline and expanding some JSON response for example.
Thanks!
@frankcalise as per requested |
One last thing, though. Screen.Recording.2025-03-12.at.13.00.08.movLooking into it right now. |
@frankcalise @coolsoftwaretyler Now we're talking! Screen.Recording.2025-03-12.at.13.26.39.mov |
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.
Awesome work, thanks for the extra thorough checks
Please verify the following:
yarn build-and-test:local
passesREADME.md
(or relevant documentation) has been updated with your changesDescribe your PR
Added a light theme and logic that switches themes according to system preferences.