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): export and announcement modal for PD 8.1 #14870

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Apr 11, 2024

closes AUTH-8 AUTH-9

Overview

Add announcement and update export modal. App version 7.2.0 or higher is needed in order to match the pipette collision warnings that were introduced in App 7.2

Screenshot 2024-04-11 at 12 01 09 Screenshot 2024-04-11 at 12 01 31

Test Plan

check the 2 modals -> first opening PD and then exporting a protocol

Changelog

  • add new announcement and text
  • update export modal
  • add test coverage for blocking hint

Review requests

see test plan

Risk assessment

low

@jerader jerader requested a review from a team as a code owner April 11, 2024 16:04
{
announcementKey: 'customParamsAndMultiTipAndModule8.1',
image: (
// TODO(jr, 4/11/24): add image to announcement modal
Copy link
Collaborator Author

@jerader jerader Apr 11, 2024

Choose a reason for hiding this comment

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

i actually don't know if we need/want an image here so i'll revisit it later when Felix has signed off on PD 8.1

Copy link
Contributor

Choose a reason for hiding this comment

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

Lots of text, and no obvious associated visual. Skipping seems fine to me.

Comment on lines 47 to 52
"body1": "Introducing the {{pd}} 8.1. Starting today, you will be able to:",
"body2": "Customize blowout flowRate speed and height",
"body3": "Customize aspirate, dispense, and mix <i>'x'</i> and <i>'y'</i> well positioning",
"body4": "Assign up to 3 types of tipracks to a single pipette",
"body5": "For the Flex only; add up to 7 Temperature Modules on the deck at the same time",
"body6": "All protocols require {{app}} version <strong> 7.2+ </strong> to run."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can you review the copy here when you have a minute @ecormany?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"body1": "Introducing the {{pd}} 8.1. Starting today, you will be able to:",
"body2": "Customize blowout flowRate speed and height",
"body3": "Customize aspirate, dispense, and mix <i>'x'</i> and <i>'y'</i> well positioning",
"body4": "Assign up to 3 types of tipracks to a single pipette",
"body5": "For the Flex only; add up to 7 Temperature Modules on the deck at the same time",
"body6": "All protocols require {{app}} version <strong> 7.2+ </strong> to run."
"body1": "Introducing {{pd}} 8.1. Starting today, you will be able to:",
"body2": "Customize blowout speed and height.",
"body3": "Adjust horizontal position within a well when aspirating, dispensing, or mixing.",
"body4": "Assign up to three types of tip racks to a single pipette.",
"body5": "Add multiple Temperature Modules to the deck (Flex only).",
"body6": "All protocols require {{app}} version <strong>7.2.0 or later</strong> to run."

Double-check me: is it 7.2.0 for the required robot software?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are pipette collision logic updates that were added in App 7.2.0 that will be mirrored in PD 8.1. Otherwise, all the added functionality in this release has been supported in App prior to App 7.2.0

@jerader jerader requested review from koji and ncdiehl11 and removed request for a team April 11, 2024 16:08
@koji
Copy link
Contributor

koji commented Apr 11, 2024

The changes look good to me.

One quick question about the first modal.
I see the scrollbar space in the first modal but your screenshot doesn't have it. Because of screenshot's area?

Screenshot 2024-04-11 at 12 17 12 PM

@koji koji requested a review from ecormany April 11, 2024 16:28
@jerader
Copy link
Collaborator Author

jerader commented Apr 11, 2024

One quick question about the first modal. I see the scrollbar space in the first modal but your screenshot doesn't have it. Because of screenshot's area?

@koji , oh wow good catch. i did not see a scrollbar from my end! i can add a css prop to make sure the scrollbar is always invisible.

@jerader
Copy link
Collaborator Author

jerader commented Apr 11, 2024

One quick question about the first modal. I see the scrollbar space in the first modal but your screenshot doesn't have it. Because of screenshot's area?

@koji , oh wow good catch. i did not see a scrollbar from my end! i can add a css prop to make sure the scrollbar is always invisible.

@koji i decided not to mess with the css since its using the legacy Modal component that a handful of stuff still uses. i'll leave the scrollbar visible for now. Hopefully we can make everything look much nicer for PD 8.2 since it'll include the redesign 🤞

@koji
Copy link
Contributor

koji commented Apr 11, 2024

One quick question about the first modal. I see the scrollbar space in the first modal but your screenshot doesn't have it. Because of screenshot's area?

@koji , oh wow good catch. i did not see a scrollbar from my end! i can add a css prop to make sure the scrollbar is always invisible.

@koji i decided not to mess with the css since its using the legacy Modal component that a handful of stuff still uses. i'll leave the scrollbar visible for now. Hopefully we can make everything look much nicer for PD 8.2 since it'll include the redesign 🤞

that makes sense. At least changes look good to me.
Once this PR gets feedback from Ed, it's good to go.

Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

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

lgtm

@koji
Copy link
Contributor

koji commented Apr 11, 2024

@jerader could you take a look into e2e test?

@jerader
Copy link
Collaborator Author

jerader commented Apr 11, 2024

@jerader could you take a look into e2e test?

@koji oops ya, its because the announcement modal text changed. i'll fix it

@jerader jerader requested a review from a team as a code owner April 11, 2024 17:54
@jerader jerader removed the request for review from a team April 11, 2024 18:01
Copy link
Contributor

@ecormany ecormany left a comment

Choose a reason for hiding this comment

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

Left some copy suggestions. This looks like an exciting release! 🚀

{
announcementKey: 'customParamsAndMultiTipAndModule8.1',
image: (
// TODO(jr, 4/11/24): add image to announcement modal
Copy link
Contributor

Choose a reason for hiding this comment

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

Lots of text, and no obvious associated visual. Skipping seems fine to me.

Comment on lines 47 to 52
"body1": "Introducing the {{pd}} 8.1. Starting today, you will be able to:",
"body2": "Customize blowout flowRate speed and height",
"body3": "Customize aspirate, dispense, and mix <i>'x'</i> and <i>'y'</i> well positioning",
"body4": "Assign up to 3 types of tipracks to a single pipette",
"body5": "For the Flex only; add up to 7 Temperature Modules on the deck at the same time",
"body6": "All protocols require {{app}} version <strong> 7.2+ </strong> to run."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"body1": "Introducing the {{pd}} 8.1. Starting today, you will be able to:",
"body2": "Customize blowout flowRate speed and height",
"body3": "Customize aspirate, dispense, and mix <i>'x'</i> and <i>'y'</i> well positioning",
"body4": "Assign up to 3 types of tipracks to a single pipette",
"body5": "For the Flex only; add up to 7 Temperature Modules on the deck at the same time",
"body6": "All protocols require {{app}} version <strong> 7.2+ </strong> to run."
"body1": "Introducing {{pd}} 8.1. Starting today, you will be able to:",
"body2": "Customize blowout speed and height.",
"body3": "Adjust horizontal position within a well when aspirating, dispensing, or mixing.",
"body4": "Assign up to three types of tip racks to a single pipette.",
"body5": "Add multiple Temperature Modules to the deck (Flex only).",
"body6": "All protocols require {{app}} version <strong>7.2.0 or later</strong> to run."

Double-check me: is it 7.2.0 for the required robot software?

@jerader jerader merged commit fa6066f into edge Apr 11, 2024
14 checks passed
@jerader jerader deleted the pd_announcement-modal-8.1 branch April 11, 2024 18:34
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.

3 participants