Open samreid opened 2 years ago
I notified the team over slack dev-public:
I’ll be adding support for React in https://github.com/phetsims/chipper/issues/1268, it has the potential to disrupt transpiling, linting and type checking, please let me know if you discover problems.
I notified slack:
For developers: I pushed support for react, please pull-all, then npm install in chipper and restart your transpiler process at your convenience. Please report problems in https://github.com/phetsims/chipper/issues/1268
Anyone want to volunteer for code reviewing that issue? I’ll mark it for dev meeting in case nobody volunteers before then.
From 6/16/22 dev meeting:
PSA delivered! @chrisklus is going to review.
For the review, can you please check the following:
@marlitas and I took a look through the above commits and files. As far as we can see, things are looking good. However, we do feel a little out of our comfort zone in regards to using React with Promises, so this case is not what we're used to.
We did notice that some aspects of CredentialsDialog are still using JavaScript patterns and could be done in a more React-like way, but what is there is working fine so it feels like it's up to developer preference. For example:
submitButton
), React has something called refs.You could use state to control the visibility of things after they've been added to the DOM instead of manually removing them like:
const node = document.getElementById( 'credentialsDialog' )!;
node.parentElement!.removeChild( node );
Clearly none of these things are needed, because everything is working as is, but @marlitas and I were just noticing that it feels like React is not being used to its full potential. It seems like the main improvement of this file is the addition of JSX. Perhaps with some of the changes above, React will make scaling easier when we make components that are more complicated and state-dependent.
Excellent, thanks for the review!
getElementById
but I'm not sure how to implement it.Maybe it would be best to schedule collaboration for next steps for this issue. I'll send a slack message about scheduling.
This patch is not working, but I wanted to leave these changes here as a placeholder before I lose them - was trying out using refs and events a couple days ago. It may be a good starting point for collaboration.
I'm not planning more time for this until we schedule a sprint or collaboration. Unassigning for now.
In https://github.com/phetsims/phet-io-wrappers/issues/440 we want to create a function that creates a DOM element dialog for entering username and password. Following https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/password we wanted to create code like this:
Which led to code like https://github.com/phetsims/phet-io-wrappers/commit/bae01d791d6e858ed6c974e9cfd80138814cb722
When we faced similar problems in PhetioElementView, we ended up creating our own utility framework for creating dom elements, like:
But it seems like React would be preferable to either of these options, and would yield more readable, maintainable code. For example, here is the credentials dialog using react:
Now that we already have babel in our toolchain, adding support for react seems feasible. This will require changes in transpiling, linting and type checking.