Skip to content

Conversation

@mercedesb
Copy link
Contributor

@mercedesb mercedesb commented May 16, 2024

I rule and have completed some work on Case Manager that's ready for review!

This pull request makes the following changes:

  • Uses our existing JS bundler, esbuild, via our existing bundling system jsbundling-rails
    • Pros: Keeps things simple, doesn't introduce any new tools, isn't Webpacker, and leaves the door open if we want to switch from esbuild to something else in the future
  • Add React and create a mount function to allow us to have multiple mount points
    • Pros: we don't have to go all-in on a SPA architecture and can stick with Rails for 90% of our use cases
  • Create a reusable ViewComponent, called ReactComponent, to make rendering React components in our erb files easy
    • Pros: Keeps Ruby code in Ruby and more easily testable. If we have to do fancy data munging before passing props, we can inherit from this base class and add that logic to the private props method
  • Migrated our existing toggle call list javascript to React to show how this would work in practice
    • Pros: React is familiar to most of our volunteers. And with this approach, we don't have to do one giant migration. We can bite it off in chunks when we have the appetite and capacity for it.

(If there are changes to the views, please include a screenshot so we know what to look for!)

image image

For reviewer:

  • Adjust the title to explain what it does for the notification email to the listserv.
  • Tag this PR:
    • feature if 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.
    • dependencies if it contains library upgrades or similar. This is anything that upgrades any dependency, such as a Gemfile update or npm package upgrade.
  • If it contains neither, no need to tag this PR.

Copy link
Member

@colinxfleming colinxfleming left a 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" }
Copy link
Member

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)

Copy link
Contributor Author

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

@mercedesb
Copy link
Contributor Author

mercedesb commented May 18, 2024

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 i18n-js Ruby gem to auto-generate JSON translation files from our existing YML translation files. And then we're using the companion i18n-js JS package to read and apply the translations. Even better news is that we can use these translations in non-React too if we have a need for that.

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 ignore_unused config but maybe we want to make a special js root in the translations so we can pattern match in ignore_unused so we don't have to manually add to this array 🤔

@mercedesb mercedesb force-pushed the POC/js-upgrade branch 3 times, most recently from e4fc4b8 to 4d734d7 Compare May 18, 2024 22:51
@mercedesb
Copy link
Contributor Author

Closing this in favor of #3204 so we can reference any example code in here later if we want

@mercedesb mercedesb closed this May 25, 2024
@lomky lomky added this to the React Autosave milestone Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants