-
Notifications
You must be signed in to change notification settings - Fork 406
save and load theme using settings api #4433
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
Conversation
resolves #4432 |
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.
First of all, thanks for your work!
It's always great to see people engage in the process with fresh ideas and perspectives - I really appreciate it. 🙏 🍺
Second, apologies for the delay.
Third, now that the requirements are clear, the changes make sense and look good overall. 🚀
I just have a few comments related to code style. Here's what I suggest:
- Rebase onto the latest main
- Go through the comments
- Add some unit tests
I'm happy to help with any (or all) of these steps - just let me know!
Restructured code as suggested |
@yashschandra I opened a PR with a few additional tweaks and polish to complement your changes. yashschandra#1 |
Apply code style rules on Theme loading PR
@pawelangelow Merged your changes. Also removed unnecessary update settings call. Please check now. |
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.
Thanks for your time and cooperation on this PR! 🙏
No description provided.