-
Notifications
You must be signed in to change notification settings - Fork 242
Proof of concept: Add ability to migrate existing JS to React #3199
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
colinxfleming
left a comment
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.
@mercedesb I really gotta hand it to you on this, this is an incredibly minimal adjustment to existing codepaths that opens up a lot of freedom for us. Feels like an extremely viable approach to me. I have a question about usage of i18n for our spanish speaking CMs and I'd like @lomky 's opinion too but I can really see us using this with very minimal changes.
|
|
||
| return ( | ||
| <button className="btn btn-link" onClick={toggleCallLog}> | ||
| { showOldCalls ? "Limit list" : "View all calls" } |
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.
is it possible to respect i18n here, or is that a bridge too far? i18next/react-i18next#909 seems to hint that there's something in view_component that will let you get away with this. (though in this case you have found one of the few spots in the app that didn't get translated ha, so that's our mistake)
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.
Def not a bridge too far, I went ahead and pushed up some commits
|
I pushed up some commits to add JS i18n. I chose not to go with react-18next so that we aren't super tightly coupled to the React ecosystem. I love React but given the current state of our JS, this just felt smart. And I wanted a solution that means we're not maintaining translations in multiple places. This uses the So now we can keep all of our rails-i18n yml files as the source of truth and it'll autogenerate our JSON translations that i18n-js needs. I wrote a little reusable hook for us. This seems like a small, simple solution keeping in line with what we've liked so far about what we have. And I went ahead and translated that toggle button. My Spanish is not great (just a few years in HS) but I think it's right. The build broke due to unused translations... I'm not totally sure the best way to handle those. I added these 2 keys explicitly to the i18n-tasks |
e4fc4b8 to
4d734d7
Compare
|
Closing this in favor of #3204 so we can reference any example code in here later if we want |
I rule and have completed some work on Case Manager that's ready for review!
This pull request makes the following changes:
esbuild, via our existing bundling systemjsbundling-railsesbuildto something else in the futuremountfunction to allow us to have multiple mount pointsViewComponent, calledReactComponent, to make rendering React components in our erb files easypropsmethod(If there are changes to the views, please include a screenshot so we know what to look for!)
For reviewer:
featureif it contains a feature, fix, or similar. This is anything that contains a user-facing fix in some way, such as frontend changes, alterations to backend behavior, or bug fixes.dependenciesif it contains library upgrades or similar. This is anything that upgrades any dependency, such as a Gemfile update or npm package upgrade.