refactor(app): refactor command text out of CommandText #15708
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Decouple the logic that generates command text from the component that renders it.
|
Tagging auth for visibility as well, since this impacts run preview stuff (which maybe is AUTH or maybe it's EXEC idk)...I eventually figured out how to tag them 😎 |
sfoster1
approved these changes
Jul 18, 2024
app/src/molecules/Command/hooks/useCommandTextString/utils/getCommentCommandText.ts
Outdated
Show resolved
Hide resolved
app/src/molecules/Command/hooks/useCommandTextString/utils/getDirectTranslationCommandText.ts
Show resolved
Hide resolved
jerader
reviewed
Jul 19, 2024
app/src/molecules/Command/hooks/useCommandTextString/utils/getConfigureForVolumeCommandText.ts
Outdated
Show resolved
Hide resolved
jerader
reviewed
Jul 19, 2024
...rc/molecules/Command/hooks/useCommandTextString/utils/getConfigureNozzleLayoutCommandText.ts
Outdated
Show resolved
Hide resolved
jerader
reviewed
Jul 19, 2024
app/src/molecules/Command/hooks/useCommandTextString/utils/getMoveToWellCommandText.ts
Show resolved
Hide resolved
jerader
reviewed
Jul 19, 2024
app/src/molecules/Command/hooks/useCommandTextString/utils/getPrepareToAspirateCommandText.ts
Outdated
Show resolved
Hide resolved
mjhuff
force-pushed
the
app_refactor-command-text
branch
from
f7936ca to
b8e53a1
Compare
mjhuff
added a commit
that referenced
this pull request
Jul 31, 2024
Explicitly handle the null command case to prevent warnings. The intention of useCommandTextString after #15708 is to support null commands, so this change genuinely implements the desired behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes EXEC-575
Overview
Works towards EXEC-574.
Decouple the logic that generates command text from the component that renders it. There was a good bit of cruft, translation files were in different places instead of the correct place, and a large number of missing string generator util functions were missing, so this PR adds/cleans up those instances.
There's one small non-functional change: the primary string generator hook,
useCommandTextString, now supports an optionalCommandTextData, whereas this variable was required before. Half the time we were accounting for thenullcase when string building, and half the time we weren't, so let's just make all string builders handle the null case and makeuseCommandTextStringa bit more flexible (which will come in handy with Error Recovery, soon!) .Test Plan
CommandText, which is not the best place for them now, but I've added a TODO to clean up the testing later.Risk assessment
low