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(protocol-designer): update UI for liquids toolbox #17342

Merged
merged 8 commits into from
Jan 27, 2025

Conversation

ncdiehl11
Copy link
Collaborator

Overview

Modify logic for saving add liquids form, displaying errors, and behavior on confirmation button of toolbox if a form is unsaved.

Closes AUTH-1368

Test Plan and Hands on Testing

  • add or modify liquids for a labware
  • try to save form with a required field missing and verify error displays
  • try to save a form with a volume out of range (0 or greater than max well volume) and verify error displays
  • with form open (errors or not), click "Done" button at bottom of toolbox, and verify that you are called to action to finish the unsaved form
  • verify that "clear selected wells" button is present and clears selected wells
  • verify that "cancel" resets form state

Changelog

  • introduce TertiaryButton to PD atoms (should probably move this to components eventually)
  • modify form logic for new liquid
  • fix liquid card style

Review requests

see test plan

Risk assessment

low

Modify logic for saving add liquids form, displaying errors, and behavior on confirmation button of toolbox if a form is unsaved.

Closes AUTH-1368
@ncdiehl11 ncdiehl11 self-assigned this Jan 24, 2025
@ncdiehl11 ncdiehl11 requested review from jerader and koji January 24, 2025 19:25
@ncdiehl11
Copy link
Collaborator Author

need to resolve cypress test with @alexjoel42

@ncdiehl11 ncdiehl11 marked this pull request as ready for review January 24, 2025 20:29
@ncdiehl11 ncdiehl11 requested a review from a team as a code owner January 24, 2025 20:29
@ncdiehl11 ncdiehl11 removed the request for review from a team January 24, 2025 20:29
@alexjoel42
Copy link
Contributor

Probably failing because the below is covered by my imprecise well-selection cursor.

cy.get('input[name="volume"]').type('150')' --> cy.get('input[name="volume"]').type('150', { force: true })
Should unblock it and fix the test temporarily

`,
}

export const TertiaryButton = styled(Btn)<{
Copy link
Collaborator

@jerader jerader Jan 27, 2025

Choose a reason for hiding this comment

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

why can't this be in components already?

Copy link
Collaborator Author

@ncdiehl11 ncdiehl11 Jan 27, 2025

Choose a reason for hiding this comment

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

Should i move it there? there is currently a tertiary button in app but not components @jerader

Copy link
Contributor

Choose a reason for hiding this comment

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

if this component is the same as tertiary button in app, moving it to component is the best way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately it required some changes from the app directory. My suggestion is that we address this component and unify in components library after this release, when we have a bit more time to discuss with design the definitive styles (the app tertiary button is used all over the place, and I'm hesitant to make a big change to it at this point). Let me know what you guys think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i sort of think we should move this into components now and then unify components and app later.

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 I see. That works for me

Copy link
Contributor

@koji koji Jan 27, 2025

Choose a reason for hiding this comment

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

i agree with Nick. tertiary button is used a lot in app and seems that the other team is busy for 830.
the change may have a big impact on 830 release.

@@ -370,7 +370,7 @@ const executeAction = (action: Actions | UniversalActions): void => {
cy.contains('My liquid!').click() // Action for clicking 'My liquid!'
break
case Actions.SetVolumeAndSaveforWells:
cy.get('input[name="volume"]').type(`150`) // Set volume
cy.get('input[name="volume"]').type('150', { force: true }) // Set volume
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did we need to add this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will let @alexjoel42 chime in here. there was an issue with cypress thinking the input element was blocked

@ncdiehl11 ncdiehl11 requested a review from jerader January 27, 2025 17:56
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.

lgtm after we move TertiaryButton into Components 🚀

@ncdiehl11 ncdiehl11 requested a review from a team as a code owner January 27, 2025 18:24
@ncdiehl11 ncdiehl11 merged commit 40c927f into edge Jan 27, 2025
36 checks passed
@ncdiehl11 ncdiehl11 deleted the feat_pd-liquids-form branch January 27, 2025 19:10
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.

4 participants