Added accesibility for default system theme to p5.js-web-editor #3371
Conversation
raclim
added
Area:Accessibility
|
To check the checkbox you can add |
|
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 |
|
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 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 That said, I do think the inclusion of |
|
I've replaced react-helmet-async with react-helmet to simplify the setup and reduce dependencies. |
| @@ -1,5 +1,5 @@ | |||
| import React, { useEffect, useState } from 'react'; | |||
| import Helmet from 'react-helmet'; | |||
| import { Helmet } from 'react-helmet'; | |||
There was a problem hiding this comment.
Since Helmet is currently a default export in this file, could we preserve that or is there a specific reason to update it to be a named one?
There was a problem hiding this comment.
The codebase uses named imports consistently, so introducing a default export would create inconsistency. While there's no technical limitation, named exports also align better with TypeScript, especially as the organization is migrating to it.
I'd love to hear your thoughts on this
| @@ -1,7 +1,7 @@ | |||
| import axios from 'axios'; | |||
| import PropTypes from 'prop-types'; | |||
| import React, { useEffect, useState } from 'react'; | |||
| import Helmet from 'react-helmet'; | |||
| import { Helmet } from 'react-helmet'; | |||
There was a problem hiding this comment.
Same with this file.
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:
npm run lint)npm run test)developbranch.Fixes #3061