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): add submerge for aspirate and dispense #17368

Open
wants to merge 13 commits into
base: edge
Choose a base branch
from

Conversation

syao1226
Copy link
Collaborator

@syao1226 syao1226 commented Jan 28, 2025

re AUTH-904, AUTH-905

Overview

In this PR, I added a Submerge section for aspirating and dispensing in the transfer step, but only when the liquid classes feature flag is enabled.

Test Plan and Hands on Testing

-Enable the liquid classes feature flag in the settings
-Add a transfer step and verify that the submerge field appears for both aspirating and dispensing

Changelog

-Added new submerge fields to 8_5_0 migration

Review requests

Risk assessment

@syao1226 syao1226 changed the title feat(protocol-designer): add submerge for aspirate feat(protocol-designer): add submerge for aspirate and dispense Jan 28, 2025
@jerader
Copy link
Collaborator

jerader commented Jan 28, 2025

Hey Shiyao! Don't forget to put the work in the move liquid tools behind the liquid classes feature flag: getEnableLiquidClasses is the reducer.

Additionally, the cypress migrations test will fail because we need to migrate the new submerge form fields. I am planning to create a 8.5.0 migration file after @ncdiehl11 cuts the 8.4.0 release branch. But you can add the new fields to that migration file once it is created.

@syao1226 syao1226 marked this pull request as ready for review January 28, 2025 20:43
@syao1226 syao1226 requested a review from a team as a code owner January 28, 2025 20:43
@ncdiehl11
Copy link
Collaborator

Hey Shiyao! Don't forget to put the work in the move liquid tools behind the liquid classes feature flag: getEnableLiquidClasses is the reducer.

Additionally, the cypress migrations test will fail because we need to migrate the new submerge form fields. I am planning to create a 8.5.0 migration file after @ncdiehl11 cuts the 8.4.0 release branch. But you can add the new fields to that migration file once it is created.

Hey @jerader, just confirming that 8.4.0 has been cut so you can safely merge the migration file to edge. No rush but ping us when you do.

@jerader
Copy link
Collaborator

jerader commented Jan 29, 2025

Hey Shiyao! Don't forget to put the work in the move liquid tools behind the liquid classes feature flag: getEnableLiquidClasses is the reducer.
Additionally, the cypress migrations test will fail because we need to migrate the new submerge form fields. I am planning to create a 8.5.0 migration file after @ncdiehl11 cuts the 8.4.0 release branch. But you can add the new fields to that migration file once it is created.

Hey @jerader, just confirming that 8.4.0 has been cut so you can safely merge the migration file to edge. No rush but ping us when you do.

@ncdiehl11, since Shiyao added a few new fields to the formData, we need to add those to a migration file. I have a Pr up that adds the 8_5_0 migration file that Shiyao can use once it is merged in. Hopefully i can get my Pr merged in today.

protocol-designer/src/form-types.ts Show resolved Hide resolved
`${tab}_delay_seconds`,
mappedErrorsToField
)}
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'll need to add our position field/modal here like designs specify for retract (see comment)

@ncdiehl11
Copy link
Collaborator

Generally looking good! I'm going to mark as "do not merge" because in addition to addressing these comments, we need to make sure we add the migration as commented above once this merges

@ncdiehl11 ncdiehl11 added blocked Ticket or PR is blocked by other work DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available labels Jan 29, 2025
@jerader
Copy link
Collaborator

jerader commented Jan 30, 2025

Just merged in my touch tip refactor PR, so you should be able to add to the 8_5_0 migration now @syao1226 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authorship blocked Ticket or PR is blocked by other work DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants