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): create multiSlideout and plug into ChooseRobotSlideout #14649

Merged
merged 4 commits into from
Mar 13, 2024

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Mar 13, 2024

closes https://opentrons.atlassian.net/browse/AUTH-97

Overview

This created the MultiSlideout component and then plugs it into the ChooseRobotSlideout

Test Plan

in the app, turn on the RTP ff, click to start protocol setup. the choose robot slideout should have 2 steps. the first step should be completed and the 2nd step should have a todo. The buttons should match designs

Changelog

  • create MultiSlideout and modify Slideout to accommodate multi steps
  • modify ChooseRobotSlideout and ChooseRobotToRunProtocolSlideout
  • fix tests

Review requests

see test plan

Risk assessment

low

@jerader jerader requested a review from a team as a code owner March 13, 2024 15:59
@jerader jerader requested review from koji, a team and ncdiehl11 and removed request for a team March 13, 2024 15:59
Comment on lines 21 to 53
const Children = (
<React.Fragment>
<StyledText
fontWeight={TYPOGRAPHY.fontWeightSemiBold}
fontSize={TYPOGRAPHY.fontSizeP}
>
{firstPage ? 'first page body' : 'second page body'}
</StyledText>

<PrimaryBtn
marginTop="28rem"
onClick={togglePage}
backgroundColor={COLORS.blue50}
textTransform={TYPOGRAPHY.textTransformNone}
>
<StyledText
fontWeight={TYPOGRAPHY.fontWeightRegular}
fontSize={TYPOGRAPHY.fontSizeP}
>
{firstPage ? 'Go to Second Page' : 'Go to First Page'}
</StyledText>
</PrimaryBtn>
</React.Fragment>
)

return (
<MultiSlideout
{...args}
children={Children}
currentStep={firstPage ? 1 : 2}
/>
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

From the error message, we will need to change something like below.
tested the following code and it worked the same as you wrote on Storybook.

Suggested change
const Children = (
<React.Fragment>
<StyledText
fontWeight={TYPOGRAPHY.fontWeightSemiBold}
fontSize={TYPOGRAPHY.fontSizeP}
>
{firstPage ? 'first page body' : 'second page body'}
</StyledText>
<PrimaryBtn
marginTop="28rem"
onClick={togglePage}
backgroundColor={COLORS.blue50}
textTransform={TYPOGRAPHY.textTransformNone}
>
<StyledText
fontWeight={TYPOGRAPHY.fontWeightRegular}
fontSize={TYPOGRAPHY.fontSizeP}
>
{firstPage ? 'Go to Second Page' : 'Go to First Page'}
</StyledText>
</PrimaryBtn>
</React.Fragment>
)
return (
<MultiSlideout
{...args}
children={Children}
currentStep={firstPage ? 1 : 2}
/>
)
}
return (
<MultiSlideout {...args} currentStep={firstPage ? 1 : 2}>
<StyledText as="p">
{firstPage ? 'first page body' : 'second page body'}
</StyledText>
<PrimaryBtn
marginTop="28rem"
onClick={togglePage}
backgroundColor={COLORS.blue50}
textTransform={TYPOGRAPHY.textTransformNone}
>
<StyledText as="p">
{firstPage ? 'Go to Second Page' : 'Go to First Page'}
</StyledText>
</PrimaryBtn>
</MultiSlideout>
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh ya i fixed this in my last commit, thank you!!

@koji koji self-requested a review March 13, 2024 16:51
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!
thank you for adding a storybook!

@jerader jerader merged commit d5095bb into edge Mar 13, 2024
20 checks passed
@jerader jerader deleted the app_multi-page-slideout branch March 13, 2024 17:10
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.

2 participants