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(odd): Large Button #12317

Merged
merged 3 commits into from
Mar 20, 2023
Merged

feat(odd): Large Button #12317

merged 3 commits into from
Mar 20, 2023

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Mar 20, 2023

closes RAUT-351

Overview

Creates component, story and test for Large Button in ODD

Test Plan

examine the 3 types of buttons + the disabled state on storybook, should match figma

Changelog

  • create ODD LargeButton component, test and storybook

Review requests

  • see test plan

Risk assessment

low

@jerader jerader requested a review from a team as a code owner March 20, 2023 13:30
@jerader jerader requested review from brenthagen, ewagoner, koji and a team and removed request for a team and brenthagen March 20, 2023 13:30
@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #12317 (e4f7c63) into edge (055fafa) will decrease coverage by 0.31%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12317      +/-   ##
==========================================
- Coverage   73.73%   73.42%   -0.31%     
==========================================
  Files        2215     1485     -730     
  Lines       61086    48850   -12236     
  Branches     6299     2984    -3315     
==========================================
- Hits        45041    35869    -9172     
+ Misses      14589    12527    -2062     
+ Partials     1456      454    -1002     
Flag Coverage Δ
app 46.43% <ø> (-25.84%) ⬇️

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

see 730 files with indirect coverage changes

onClick: () => void
buttonType: LargeButtonTypes
buttonText: React.ReactNode
disabled?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Context section, this large button component uses different icons for each button.
I think we need to pass IconName as a props.

Suggested change
disabled?: boolean
disabled?: boolean
iconName: IconName

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops good catch! I will add that, thank you!

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.

Mostly looks good to me but from the Figma page, we will need to pass an icon name.

@jerader jerader requested a review from koji March 20, 2023 15:27
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.

Tested passing an iconName to this component and it worked as expected.
⏺️

@jerader jerader merged commit dacc8cc into edge Mar 20, 2023
@jerader jerader deleted the odd_large-button-creation branch March 20, 2023 17:39
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