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

refactor(app): update sectionList text to reflect new design #9223

Merged
merged 2 commits into from
Jan 11, 2022

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Jan 10, 2022

closes #9159

Overview

This PR updates the text on the steps in sectionList

Screen Shot 2022-01-10 at 13 55 37

Changelog

  • utilize i18n plural to change text depending on if the protocol uses 1 or 2 pipettes
  • update component and test

Review requests

  • I only checked how it looks visually when you need only 1 pipette (see above photo). Check to see how it looks if you need 2 pipettes.
  • makes sure it matches the design (see ticket for details)

Risk assessment

low, behind ff

@jerader jerader requested a review from a team as a code owner January 10, 2022 20:01
@jerader jerader requested review from smb2268 and removed request for a team January 10, 2022 20:01
@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #9223 (e44e344) into edge (03e3c55) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge    #9223   +/-   ##
=======================================
  Coverage   74.89%   74.89%           
=======================================
  Files        1882     1882           
  Lines       50167    50167           
  Branches     4938     4938           
=======================================
  Hits        37571    37571           
  Misses      11704    11704           
  Partials      892      892           
Flag Coverage Δ
app 70.63% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

Comment on lines 102 to 104
count: sections.includes('SECONDARY_PIPETTE_TIPRACKS')
? 2
: 1,
Copy link
Member

Choose a reason for hiding this comment

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

the use of count as a pluralizer here is confusing to me. we tell i18n to pluralize the entry when there the user is in a two pipette flow, but it's hard to tell what that actually maps to. the pluralizer is really meant to change things like one item to two items. here we aren't actually pluralizing anything (as far as I can tell.). also it looks like depending on whether we're in a 1 pipette or 2 pipette workflow, the wording should be as follows:

1 pip flow:
Step 1: Check tipracks with Pipette and pick up a tip
// you actually have: Check tip racks with {{primary_mount}} Pipette and pick up a tip
Step 2: Check remaining labware with Pipette and tip
// you actually have: Check remaining labware with {{secondary_mount}} and tip
2 pip flow:
Step 2: Check tipracks with Right Pipette and pick up a tip
// you actually have: Check tip racks with {{primary_mount}} Pipette
Step 3: Check remaining labware with Right Pipette and tip
// this one looks correct

tbh i am confused because the image and text on the ticket say 2 different things... @emilywools is what jethary has here what you intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh good call, I was following what the images show in the ticket @emilywools please LMK if that's correct?

@shlokamin Also, I'm confused as to how this isn't a way to use count as a pluralizer? We have several other areas in the repo where i18n plural is used to completely change the text: for example in protocol_setup.json there is:

"module_setup_step_description": "Plug in and turn on the required module via the OT-2 USB Port. Place the module as shown in the deck map."

"module_setup_step_description_plural": "Plug in and turn on the required modules via the OT-2 USB Ports. Place the modules as shown in the deck map."

Copy link
Member

@shlokamin shlokamin Jan 10, 2022

Choose a reason for hiding this comment

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

the example you have above looks like it's just changing module => modules and port => ports, which makes sense since its just pluralizing the subject nouns.

this is slightly different:

  "primary_pipette_tipracks_section": "Check tip racks with {{primary_mount}} Pipette and pick up a tip",
  "primary_pipette_tipracks_section_plural": "Check tip racks with {{primary_mount}} Pipette",

we're not really pluralizing anything here, so when I read this, I'm sort of confused as to what the singular/plural text for primary_pipette_tipracks_section means. we don't change anything from singular to plural, we're really just adding and pick up tip when it's "singular"

@shlokamin
Copy link
Member

screenshot for 2 pipettes:
Screen Shot 2022-01-10 at 5 30 49 PM

@emilywools
Copy link

hey @jerader Jethary, I think overall what you have is correct, but Shlok grabbed a 2-pipette workflow screenshot, and the "pick up a tip" belongs with step 2, not step 1. Does that make sense?

Screen Shot 2022-01-10 at 5 30 49 PM

@jerader
Copy link
Collaborator Author

jerader commented Jan 10, 2022

hey @jerader Jethary, I think overall what you have is correct, but Shlok grabbed a 2-pipette workflow screenshot, and the "pick up a tip" belongs with step 2, not step 1. Does that make sense?

Screen Shot 2022-01-10 at 5 30 49 PM

Not the same protocol that Shlok used but fixed the issue. Here is what it looks like now with 2 pipettes: @emilywools @shlokamin

Screen Shot 2022-01-10 at 18 25 08

@emilywools
Copy link

this looks good to me, thank you!

Copy link
Member

@shlokamin shlokamin left a comment

Choose a reason for hiding this comment

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

🚢

@jerader jerader merged commit 501bc09 into edge Jan 11, 2022
@jerader jerader deleted the app_lpc-update-step-text branch January 11, 2022 20:33
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.

update LPC step copy to indicate that ya pick up the tip
4 participants