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, components): wire up pause form in PD redesign #16328

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

ncdiehl11
Copy link
Collaborator

@ncdiehl11 ncdiehl11 commented Sep 23, 2024

Overview

This PR adds form functionality and UI for pause step in PD redesign. I consolidate pause hours, minutes, and seconds into a single colon-delimited time field, add errors and masking, and parse the time string when creating command arguments. I also touch several components that require styling refactors, including Toolbox, DropdownMenu, and RadioButton. Note that migrating separate pause time fields to a single field will be addressed in a future PR.

Closes AUTH-809

Test Plan and Hands on Testing

  • In PD redesign, create a protocol with a pause step
  • Navigate through 3 options and confirm that text fields match designs
  • Specifically in "Delay for an amount of time" pause type, verify expected behavior of time string formatted as hh:mm:ss

Changelog

  • add content to PauseTools component
  • add new overarching pauseTime field to pause form data (will replace pauseHours, minutes, and seconds once redesign FF is removed)
  • add maskers and errors for pauseTime text
  • add util for pulling time data from form while we have a mixture of pauseHours/Minutes/Seconds and pauseTime
  • fix styles on various components implicated here
  • add translations and update tests

Review requests

Risk assessment

This PR adds form functionality and UI for pause step in PD redesign. I consolidate pause hours,
minutes, and seconds into a single colon-delimited time field, add errors and masking, and parse the
time string when creating command arguments. I also touch several components that require styling
refactors, including `Toolbox`, `DropdownMenu`, and `RadioButton`. Note that migrating separate
pause time fields to a single field will be addressed in a future PR.

Closes AUTH-809
@ncdiehl11 ncdiehl11 marked this pull request as ready for review September 23, 2024 20:00
@ncdiehl11 ncdiehl11 requested review from a team as code owners September 23, 2024 20:00
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.

left a handful of cleanup comments but this is looking really good! nice work!

i also left some comments about the errorsToShow stuff. The bug is specifically with makeSingleEditFieldProps in protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/utils.ts. on line 259, the logic is wrong and should say const errorToShow = showErrors && errors.length > 0 ? errors.join(', ') : null. I think that will prevent errorsToShow from populating unless the field has been dirtied/selected.

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.

left a small clean up comment but after that is addressed, this LGTM! nice work with adding the new field! excited to see the pause step form come to life. Don't forget to add a ticket in the migration epic if you haven't already to remind us to migrate the old fields over.

@ncdiehl11
Copy link
Collaborator Author

ignoring brittle Cyprus test

@ncdiehl11 ncdiehl11 merged commit 786e2bd into edge Sep 25, 2024
31 of 35 checks passed
@ncdiehl11 ncdiehl11 deleted the pd_redesign-pause-stepform branch September 25, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants