refactor(components): add data id to pipette dropdown options #5822
Conversation
shlokamin
force-pushed
the
test_pipette-dropdown-option-data-attr
branch
from
a9d86e4 to
10f2118
Compare
| @@ -118,10 +118,32 @@ const PipetteNameItem = (props: PipetteNameSpecs) => { | |||
| } | |||
|
|
|||
| return ( | |||
| <> | |||
| <div | |||
There was a problem hiding this comment.
When changing this from a React.Fragment to a div, I had to make sure this div's children show up side by side. Not sure of a better way to do this than by adding flex styling here.
There was a problem hiding this comment.
Also, I don't think this breaks styling in the app, but it is used in app/src/components/ChangePipette/PipetteSelection.js
There was a problem hiding this comment.
If you wanted to get real fancy, you could use a primitive Flex here, which is a div with display: flex, and you wouldn't need to mess with any CSS
| const dataIdFormat = ( | ||
| componentName: string, | ||
| volumeClass: string, | ||
| channels: number, | ||
| displayCategory: string | ||
| ): string => { |
There was a problem hiding this comment.
Don't love having to do this but not sure of another way since we should be keeping our data-id format consistent with ComponentName_componentSelector.
Could move this out to a util but the formatting logic here is specific to the props this component has
There was a problem hiding this comment.
Down to leave this alone until we spend some more time with this and notice our patterns
add data id to pipette dropdown options #5724
shlokamin
force-pushed
the
test_pipette-dropdown-option-data-attr
branch
from
646e7bc to
d760fd3
Compare
There was a problem hiding this comment.
LGTM 🉐
Aside: I did some digging into the proper aria attributes for a non-<select> selector to see if there was a11y properties we could be hooking on, and I'm not sure our (react-)select components are super accessible at the moment! Might be a good tech-debt followup, because it's a larger thing than this specific PR
| @@ -118,10 +118,32 @@ const PipetteNameItem = (props: PipetteNameSpecs) => { | |||
| } | |||
|
|
|||
| return ( | |||
| <> | |||
| <div | |||
There was a problem hiding this comment.
If you wanted to get real fancy, you could use a primitive Flex here, which is a div with display: flex, and you wouldn't need to mess with any CSS
| const dataIdFormat = ( | ||
| componentName: string, | ||
| volumeClass: string, | ||
| channels: number, | ||
| displayCategory: string | ||
| ): string => { |
There was a problem hiding this comment.
Down to leave this alone until we spend some more time with this and notice our patterns
| @@ -44,7 +44,7 @@ describe('Protocols with Modules', () => { | |||
| .next() | |||
| .contains('None') | |||
| .click() | |||
| cy.get('div[id*="select-3-option-2-5"]').click() | |||
| cy.get('div[data-id="PipetteNameItem_p300MultiChannelGen1"]').click() | |||
There was a problem hiding this comment.
May want to omit the div here since it doesn't have any semantic meaning
use relative import Co-authored-by: Mike Cousins <[email protected]>
mcous
changed the title
overview
This PR adds a data id to pipette dropdown options.
Partially addresses #5724.
changelog
review requests
Make sure it this seems reasonable
risk assessment
Very low, just adding a data-id for testing