p5.js-web-editor
p5.js-web-editor copied to clipboard
Added accesibility for default system theme to p5.js-web-editor
I added a button in the settings dropdown menu as "system preferences" which enables default system theme of the user to the p5.js web editor. Fixes #3061
Changes: added accessibility for default system theme in the p5.js-web-editor
I have verified that this pull request:
- [x] has no linting errors (
npm run lint) - [x] has no test errors (
npm run test) - [x] is from a uniquely-named feature branch and is up to date with the
developbranch. - [x] is descriptively named and links to an issue number, i.e.
Fixes #3061
To check the checkbox you can add x between the square brackets.
Reviewed and tested the implementation of the default system theme. Verified that theme switches correctly based on system preferences across supported browsers. Ready for review and merge.
Thanks so much for working on this!
I just checked it out and can confirm that it seems to work so far! I'm not sure if this might've been mentioned somewhere already, but could you delve into why react-helmet-async is added? Was it specifically for testing?
The migration from react-helmet to react-helmet-async was primarily done for testing purposes, but not exclusively. It also helps maintain consistency between the testing and production environments. However, I realize this is a broader change that could affect the overall code workflow. I'm happy to switch back to react-helmet if you prefer.
Thanks for providing the additional context!
In the React 19 docs, there's a section regarding Metadata, which provides more native support but also recommends using a supplementary library like react-helmet. I've also seen several sources mention that react-helmet is currently deprecated, so it might be worth investigating whether react-helmet-async is the better longterm choice.
As a note and reference to the work here so far, if we do decide to switch to a different metadata library, I think we could move away from using a utility file like update-helmet.js and do a complete refactor with the new library.
That said, I do think the inclusion of react-helmet-async feels out of scope for this PR/issue, which is focused on system preference theming, and might need some further research. Would you mind removing it from this PR for now? If you're interested, feel free to dive further into this and open an issue for it!
I've replaced react-helmet-async with react-helmet to simplify the setup and reduce dependencies. Please review the changes and let me know if any adjustments are needed.
Hi @raclim I have made the changes needed, can you please review this PR. Thanks in advance
I think this overall looks good to me! The only remnant issue seems to be merge conflicts with the package-lock.json file. I'll look into resolving this, but feel free to give it a try on your end as well if you'd like!