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(app): update modal component and labware liquid modal #11216

Merged
merged 4 commits into from
Jul 29, 2022

Conversation

smb2268
Copy link
Contributor

@smb2268 smb2268 commented Jul 25, 2022

fix #11142

Overview

This PR updates our modal component to no longer depend on the BaseModal

Changelog

  1. Create ModalHeader and ModalShell that match Mel's design system and cover the needs for all currently designed modals
  2. Update Modal atom to use these when FF is on, and add in some styling to make sure we keep the styling BaseModal had for all current instances of this atom
  3. Use ModalHeader and ModalShell to finish last three design QA requests for the LiquidsLabwareDetailsModal

Review requests

  1. Should I move this to molecules or leave in atoms?
  2. Does this approach - creating ModalHeader and ModalShell that will work for all future modal creations but adding styling to keep the Modal index the same for backwards compatibility make sense? Take a look at the ticket and loom for this PR if you want more info on where the specifics are coming from
  3. Make sure labware liquid details modal matches designs completely now

Risk assessment

Low

@smb2268 smb2268 self-assigned this Jul 25, 2022
@smb2268 smb2268 requested a review from a team as a code owner July 25, 2022 21:04
app/src/atoms/Modal/index.tsx Outdated Show resolved Hide resolved
left: 0,
width: '100%',
height: '100%',
} as const
Copy link
Collaborator

Choose a reason for hiding this comment

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

just curious why all of these are as const instead of using css, like

const BASE_STYLE = css' position: POSITION_ABSOLUTE; alignItems: ALIGN_CENTER; justifyContent: JUSTIFY_CENTER; top: 0; right: 0; bottom: 0; left: 0; width: '100%'; height: '100%'; '

I guess they achieve the same thing but not sure if we have a preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pulled this from BaseModal so it's using an old pattern. I'll update it to use css!

app/src/atoms/Modal/ModalHeader.tsx Outdated Show resolved Hide resolved
app/src/atoms/Modal/index.tsx Outdated Show resolved Hide resolved
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.

I think if we follow atomic design, Modal should be in molecules since we use a few atoms and components from @opentrons/components

@smb2268 smb2268 force-pushed the app_modal-component-updates branch 2 times, most recently from 4549ca2 to d73e028 Compare July 27, 2022 16:14
@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #11216 (97e6515) into edge (0115066) will increase coverage by 0.03%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #11216      +/-   ##
==========================================
+ Coverage   73.78%   73.81%   +0.03%     
==========================================
  Files        2089     2091       +2     
  Lines       57737    58166     +429     
  Branches     5859     5973     +114     
==========================================
+ Hits        42599    42937     +338     
- Misses      13859    13929      +70     
- Partials     1279     1300      +21     
Flag Coverage Δ
app 71.40% <60.00%> (+0.21%) ⬆️

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

Impacted Files Coverage Δ
app/src/atoms/Modal/index.tsx 75.00% <ø> (-16.67%) ⬇️
...colRun/SetupLiquids/LiquidsLabwareDetailsModal.tsx 83.87% <ø> (ø)
app/src/atoms/Modal/ModalShell.tsx 58.33% <58.33%> (ø)
app/src/atoms/Modal/ModalHeader.tsx 62.50% <62.50%> (ø)
...ganisms/LabwarePositionCheck/utils/stepCreators.ts 90.38% <0.00%> (-9.62%) ⬇️
app/src/pages/AppSettings/index.tsx 66.66% <0.00%> (-8.34%) ⬇️
...ms/Devices/ProtocolRun/RunLogProtocolSetupInfo.tsx 78.66% <0.00%> (-4.27%) ⬇️
app/src/pages/Devices/ProtocolRunDetails/index.tsx 97.26% <0.00%> (-0.57%) ⬇️
...organisms/Devices/HeaterShakerWizard/TestShake.tsx 68.88% <0.00%> (-0.35%) ⬇️
... and 9 more

@smb2268
Copy link
Contributor Author

smb2268 commented Jul 27, 2022

Note for dev reviews: I removed the changes to the Modal/index.tsx and am holding off on moving it into molecules until 6.1 goes out to minimize risk. I'll make a followup ticket in Jira to tackle those after this merges! @jerader @koji

@smb2268 smb2268 requested review from koji and jerader July 27, 2022 18:35
Copy link

@mmencarelli mmencarelli left a comment

Choose a reason for hiding this comment

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

1 change requested specifically for the margins on the liquid modal.

  • Liquid cards should bleed off the bottom edge of the modal

Screen Shot 2022-07-27 at 3 06 24 PM

Comment on lines 63 to 65
data-testid={`ModalHeader_icon_close_${
typeof title === 'string' ? title : ''
}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe, to avoid data-testid="ModalHeader_icon_close_" ?

data-testid={`ModalHeader_icon_close${
  typeof title === 'string' ? `_${title}` : ''
}`}

@smb2268 smb2268 merged commit 3bd4b3a into edge Jul 29, 2022
@smb2268 smb2268 deleted the app_modal-component-updates branch July 29, 2022 16:11
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.

Design QA: Liquid Labware Modal / Generalize Modal Component
4 participants