Skip to content
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

feat(components, app): move CommandText and TimelineScrubber components #17284

Merged
merged 10 commits into from
Jan 16, 2025

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Jan 15, 2025

Overview

This PR is part of very preliminary work to explore adding the timeline scrubber to other projects other than only the app. In order to start that work, the timeline scrubber & command text needed to be accessible outside of the app so I moved those components and affected components to the components directory.

Additionally, I added step-generation as a module for components and I added i18next as a dependency, since command text has translations.

I made sure not to duplicate utils in components and the app so this reorganization should be relatively clean, the PR is just huge.

Test Plan and Hands on Testing

Smoke test everything in app/ODD that utilizes command text and command text components. Things i can think of are:

  1. annotated steps
  2. timeline scrubber (behind ff and only for flex)
  3. run log

Changelog

move protocol timeline scrubber, command text, and various utils into components directory
add step-generation as a module for components directory
add i18next dependency to components directory

Review requests

This is a large PR. In particular, it would be great if you could review how i'm sharing the i18n translation files across projects. I think it makes sense since i followed the same patterns as exporting labware/pipette json files from shared-data. But i'd like another set of eyes to look at that!

Also note: I added i18next to components. There is no wrapper since components in the components directory are decoupled from one another. The wrapper is dependent on the main projects (i.e., the app and protocol-designer and ai, etc).

UPDATE: spoke with @y3rsh and splitting up the json files in multiple folders is against the pattern that must be followed in locize. So for a quick fix, I kept protocol_command_text.json in the app and duplicated it for components for testing. We will investigate a more efficient fix in the future so we can keep 1 source of truth.

Risk assessment

med - moves a lot of things

@jerader jerader requested a review from a team as a code owner January 15, 2025 21:31
@jerader jerader requested review from ncdiehl11, koji, sfoster1, shlokamin, smb2268 and mjhuff and removed request for a team January 15, 2025 21:31
Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code overall looks good! I think I agree with the overall approach to sharing localization files, but I think we should ensure these changes do not interfere with how we currently conduct the translation process. @smb2268 do you foresee any issues with moving i18n files to shared-data when it comes to app translation?

@shlokamin
Copy link
Member

Love this — my only hesitation is putting internationalization files in shared-data. i need to think on this more, it just doesn't feel right. my gut tells me internationalization files are product specific, and it should be the responsibility of individual products to inject internationalization files into components at runtime.

i do also see the value in sharing internationalization files though. i think it's worth doing some quick research on what other orgs to to share internationalization files across products

@jerader
Copy link
Collaborator Author

jerader commented Jan 16, 2025

Love this — my only hesitation is putting internationalization files in shared-data. i need to think on this more, it just doesn't feel right. my gut tells me internationalization files are product specific, and it should be the responsibility of individual products to inject internationalization files into components at runtime.

i do also see the value in sharing internationalization files though. i think it's worth doing some quick research on what other orgs to to share internationalization files across products

I'll do some research on what other orgs do but just wanted to put it out there that without sharing these files, each product that uses the timeline scrubber would need its own versions of the command text internationalization files. Sharing them keeps 1 source of truth, which i think is especially important now that we are translating in 2 languages.

@koji
Copy link
Contributor

koji commented Jan 16, 2025

ui components and shared-data are published as an npm package so less files/less dependencies would be good.
I guess it's not ideal to have a setup where a package needs to be released every time text is added or translated, especially if it can be avoided.
famous open-source ui components don't use i18n for the ui components themselves.
But I think this would depend on your research and team's dev policy.

@jerader
Copy link
Collaborator Author

jerader commented Jan 16, 2025

ui components and shared-data are published as an npm package so less files/less dependencies would be good. I guess it's ideal to have a setup where a package needs to be released every time text is added or translated, especially if it can be avoided.

@koji i see, makes me think the shared json files should be in the components directory instead of shared-data. But then again, there are many instances of shared-data in the components directory so coupling them together even more here doesn't change much imo?

@smb2268
Copy link
Contributor

smb2268 commented Jan 16, 2025

The code overall looks good! I think I agree with the overall approach to sharing localization files, but I think we should ensure these changes do not interfere with how we currently conduct the translation process. @smb2268 do you foresee any issues with moving i18n files to shared-data when it comes to app translation?

@y3rsh I'll pass this question on to you since you're the one managing pulling these files into locize. Would it mess with the locize flow to move a few of the files into the shared-data directory?

@jerader jerader changed the title feat(components, app, shared-data): move CommandText and TimelineScrubber components feat(components, app): move CommandText and TimelineScrubber components Jan 16, 2025
Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me and keeps everyone happy for now. We'll have to make some decisions about how to handle i18n files once workspaces that aren't the app use internationalization, but this gives us some time to mull things over. Thank you for doing this!

@jerader jerader merged commit 59bcddc into edge Jan 16, 2025
59 checks passed
@jerader jerader deleted the skunk_timeline-scrubber-components branch January 16, 2025 20:29
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.

5 participants