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): add pipette render component #8414

Merged
merged 21 commits into from
Oct 4, 2021

Conversation

shlokamin
Copy link
Member

@shlokamin shlokamin commented Sep 28, 2021

Overview

This PR adds a PipetteRender component into the components library. Closes #8379

Changelog

  • Add pipette render component

Review requests

  • Head over here and give it a try! Use both single and multi channel pipettes, and look with different labware

  • Code review

Note: i had to play around with the hard coded dimensions outlined in the ticket to make them fit properly.

Risk assessment

Low

@codecov
Copy link

codecov bot commented Sep 28, 2021

Codecov Report

Merging #8414 (2ef5f49) into edge (26f8ea4) will decrease coverage by 0.06%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #8414      +/-   ##
==========================================
- Coverage   74.31%   74.24%   -0.07%     
==========================================
  Files        1672     1681       +9     
  Lines       45149    45289     +140     
  Branches     4595     4643      +48     
==========================================
+ Hits        33553    33627      +74     
- Misses      10803    10860      +57     
- Partials      793      802       +9     
Flag Coverage Δ
app 71.14% <ø> (+0.03%) ⬆️
components 47.18% <63.63%> (-0.57%) ⬇️
labware-library 50.22% <ø> (ø)
protocol-designer 44.15% <ø> (ø)

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

Impacted Files Coverage Δ
...src/hardware-sim/Pipette/PipetteRender.stories.tsx 0.00% <0.00%> (ø)
...omponents/src/testing/utils/mountWithProviders.tsx 0.00% <ø> (ø)
components/src/testing/utils/mountWithStore.ts 0.00% <ø> (ø)
...mponents/src/testing/utils/renderWithProviders.tsx 0.00% <0.00%> (ø)
...src/hardware-sim/Pipette/EightEmanatingNozzles.tsx 100.00% <100.00%> (ø)
...nents/src/hardware-sim/Pipette/EmanatingNozzle.tsx 100.00% <100.00%> (ø)
...ponents/src/hardware-sim/Pipette/PipetteRender.tsx 100.00% <100.00%> (ø)
components/src/hardware-sim/Pipette/constants.ts 100.00% <100.00%> (ø)
components/src/testing/utils/matchers.ts 83.33% <100.00%> (ø)
.../RunSetupCard/RobotCalibration/DeckCalibration.tsx 88.88% <0.00%> (-3.97%) ⬇️
... and 14 more

Comment on lines 1 to 10
{
"extends": "../../tsconfig-base.json",
"references": [],
"compilerOptions": {
"composite": true,
"emitDeclarationOnly": false,
"rootDir": ".",
"outDir": "lib"
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

had to add this because before none of the utils were actually being used within components, they were just being used by other modules in the monorepo

Copy link
Member Author

@shlokamin shlokamin Sep 29, 2021

Choose a reason for hiding this comment

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

actually, i don't think this is the right way to do this bcuz we'd have to split up code in __utils__ into src and lib (as the current code stands lib is already being generated by the typescript compiler). yuck, maybe we should just expose these utils properly out of components/src. @b-cooper @smb2268 what say y'all?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to expose the utils from components/src, but I agree with what @b-cooper said in slack that it could be helpful to come up with a new naming convention for the utils so that they're more easily distinguishable from the other exports. I'll try to come up with some ideas for what that could be

Comment on lines 32 to 34
const cy = channels === 1 ? SINGLE_CHANNEL_PIPETTE_HEIGHT / 2 : 14
const x = labwareDef.wells.A1.x - cx
const y = channels === 1 ? labwareDef.wells.A1.y - cy : -2
Copy link
Member Author

Choose a reason for hiding this comment

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

some of these magic numbers are here because we're not actually rendering the nozzles from pipette geometry, we're hard coding values. it's probably worth at least throwing these magic numbers into constants so others can follow what's going on here

getByText(
`rectangle with width ${SINGLE_CHANNEL_PIPETTE_WIDTH} and height ${SINGLE_CHANNEL_PIPETTE_HEIGHT}`
)
mockEmanatingNozzle.mockRestore()
Copy link
Member Author

Choose a reason for hiding this comment

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

oddly enough, doing a resetAllWhenMocks() or jest.restoreAllMocks() in an afterEach does not clear out calls to mockEmanatingNozzle. i'm not sure why, and dont want to go down that rabbithole right now

Comment on lines +1 to +3
.overflow {
overflow: visible;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this is unfortunately necessary because RobotCoordsForeignDiv still uses css modules, and i need to tell it to allow the circle animations to overflow

@@ -4,6 +4,7 @@ module.exports = {
setupFilesAfterEnv: [
'<rootDir>/scripts/setup-enzyme.js',
'<rootDir>/scripts/setup-global-mocks.js',
'<rootDir>/scripts/setup-global-imports.js',
Copy link
Member Author

Choose a reason for hiding this comment

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

this got deleted somehow, adding it back. once this merges i can remove all import '@testing-library/jest-dom' statements in the monorepo

@@ -11,3 +11,5 @@ export const partialComponentPropsMatcher = (argsToMatch: unknown) =>
when.allArgs((args, equals) =>
equals(args[0], expect.objectContaining(argsToMatch))
)

export const anyProps = partialComponentPropsMatcher({})
Copy link
Member Author

Choose a reason for hiding this comment

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

this dude can be used to tell your tests that your mock component can take in any props

@shlokamin
Copy link
Member Author

code cov is mad bcuz technically the storybook component isn't tested, but its not source code

@shlokamin shlokamin marked this pull request as ready for review September 28, 2021 17:27
@shlokamin shlokamin requested a review from a team as a code owner September 28, 2021 17:27
@shlokamin shlokamin requested review from b-cooper, jerader, Kadee80 and smb2268 and removed request for a team September 28, 2021 17:27
Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

This is really cool! Thanks for the sprinkle of comments explaining why you did some things. The only thing is the 8 emanating nozzles are slightly off center when you are looking at the tiprack labware. I think you mentioned that you fixed this in a commit? So not sure why it is still off center. But other than that, looks good to me. 🚀

@shlokamin
Copy link
Member Author

shlokamin commented Sep 28, 2021

the only thing is the 8 emanating nozzles are slightly off center when you are looking at the tiprack labware

oops! forgot to push, just did

Copy link
Contributor

@smb2268 smb2268 left a comment

Choose a reason for hiding this comment

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

This is really cool, Shlok! I think it looks great. The only thing I noticed was that the blue circle within the grey outline for the Opentrons 96 TIpRack looks a little above the center.
Screen Shot 2021-10-01 at 10 49 01 AM

Comment on lines 1 to 10
{
"extends": "../../tsconfig-base.json",
"references": [],
"compilerOptions": {
"composite": true,
"emitDeclarationOnly": false,
"rootDir": ".",
"outDir": "lib"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to expose the utils from components/src, but I agree with what @b-cooper said in slack that it could be helpful to come up with a new naming convention for the utils so that they're more easily distinguishable from the other exports. I'll try to come up with some ideas for what that could be

@shlokamin shlokamin force-pushed the components_add-pipette-render-component branch from e95f6af to f04a5e9 Compare October 1, 2021 15:44
@shlokamin shlokamin force-pushed the components_add-pipette-render-component branch from 81c3374 to 99c2256 Compare October 4, 2021 18:58
@shlokamin shlokamin merged commit 8c008c4 into edge Oct 4, 2021
@shlokamin shlokamin deleted the components_add-pipette-render-component branch October 4, 2021 20:57
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.

Labware Position Check: Pipette 2d render
3 participants