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

refactor(app): are you sure you want to cancel modal on desktop app #15662

Merged

Conversation

TamarZanzouri
Copy link
Contributor

Overview

closes https://opentrons.atlassian.net/browse/EXEC-553.
wire up "are you sure you want to cancel?" desktop ER modal.

Test Plan

  • Test on ODD cancel run flow and make sure the styles didnt change.
  • Tested on my local app + components.

Changelog

Mainly change style to support ODD and Desktop apps.

Risk assessment

low.

@TamarZanzouri TamarZanzouri requested review from sfoster1 and mjhuff July 15, 2024 19:29
@TamarZanzouri TamarZanzouri requested a review from a team as a code owner July 15, 2024 19:29
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

  • some code-style stuff
  • Instead of making these changes to LegacyStyledText, we should switch uses to StyledText. See the inline review for more details.

app/src/atoms/buttons/RadioButton.tsx Show resolved Hide resolved
app/src/atoms/buttons/SmallButton.tsx Outdated Show resolved Hide resolved
app/src/molecules/InterventionModal/index.tsx Outdated Show resolved Hide resolved
app/src/molecules/InterventionModal/index.tsx Outdated Show resolved Hide resolved
app/src/molecules/InterventionModal/index.tsx Outdated Show resolved Hide resolved
components/src/atoms/StyledText/LegacyStyledText.tsx Outdated Show resolved Hide resolved
components/src/ui-style-constants/typography.ts Outdated Show resolved Hide resolved
@@ -226,7 +226,7 @@ describe('StyledText', () => {
children: 'h3Bold',
}
render(props)
expect(screen.getByText('h3Bold')).toHaveStyle(
expect(screen.queryAllByText('h3Bold')[0]).toHaveStyle(
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally prefer when rendering only one element for the sake of clarity.

Suggested change
expect(screen.queryAllByText('h3Bold')[0]).toHaveStyle(
expect(screen.getByText('h3Bold')).toHaveStyle(

@@ -243,7 +243,7 @@ describe('StyledText', () => {
children: 'h4Bold',
}
render(props)
expect(screen.getByText('h4Bold')).toHaveStyle(
expect(screen.queryAllByText('h4Bold')[0]).toHaveStyle(
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
expect(screen.queryAllByText('h4Bold')[0]).toHaveStyle(
expect(screen.getByText('h4Bold')).toHaveStyle(

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we want to be changing the typography file? We really shouldn't be touching this unless design tells us to, but I could be missing something.

Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

It's close! Seems like the overflow menu might be a bit off on the desktop:

Screenshot 2024-07-18 at 2 52 43 PM

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Will test further, but definitely need to remove all changes to LegacyStyledText.tsx

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Great! LGTM once it passes checks!

Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

There's still a couple CSS issues:

  • The font color on the cancel run subtext is not black90.
  • The text isn't centered on the modal.

That being said, I don't think this is blocking. We can circle up and do touch-ups later. Thank you!

@TamarZanzouri TamarZanzouri merged commit a6348e5 into edge Jul 19, 2024
34 checks passed
@TamarZanzouri TamarZanzouri deleted the EXEC-553-desktop-wire-up-are-you-sure-you-want-to-cancel branch July 19, 2024 16:40
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