-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: Added SelectButton #7938
feat: Added SelectButton #7938
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we break this PR down further? As of writing this comment it's 1,408
code changes which is on the large side. Also a video rather than long images I think would really improve the PR description and help QA this PR
…to morph/select-button
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7938 +/- ##
==========================================
+ Coverage 39.39% 39.44% +0.05%
==========================================
Files 1220 1227 +7
Lines 29800 29861 +61
Branches 2831 2843 +12
==========================================
+ Hits 11740 11779 +39
- Misses 17371 17387 +16
- Partials 689 695 +6 ☔ View full report in Codecov by Sentry. |
…to morph/select-button
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Left a few suggestions
app/component-library/components/Select/SelectButton/foundation/SelectButtonBase.types.ts
Outdated
Show resolved
Hide resolved
app/component-library/components/Select/SelectButton/SelectButton.stories.tsx
Outdated
Show resolved
Hide resolved
app/component-library/components/Select/SelectButton/foundation/SelectButtonBase.types.ts
Show resolved
Hide resolved
app/component-library/components/Select/SelectButton/SelectButton.stories.tsx
Show resolved
Hide resolved
app/component-library/components/Select/SelectButton/SelectButton.test.tsx
Show resolved
Hide resolved
app/component-library/components/Select/SelectButton/SelectButton.types.ts
Show resolved
Hide resolved
app/component-library/components/Select/SelectButton/foundation/SelectButtonBase.tsx
Show resolved
Hide resolved
…to morph/select-button
…to morph/select-button
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A few non blocking suggestions. Still looks like the PR template hasn't been filled out adequately and missing the checklist.
app/component-library/components/Select/SelectButton/SelectButton.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: George Marshall <george.marshall@consensys.net>
Co-authored-by: George Marshall <george.marshall@consensys.net>
Co-authored-by: George Marshall <george.marshall@consensys.net>
…ton.tsx Co-authored-by: George Marshall <george.marshall@consensys.net>
Co-authored-by: George Marshall <george.marshall@consensys.net>
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 1 New issue |
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 1 New issue |
app/component-library/components/Select/SelectButton/foundation/SelectButtonBase.test.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good however, there doesn't seem to be adequate testing coverage for this component? You are not testing all of the sizes, isDisabled, isDanger props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing as other mobile components have very light testing coverage and there are no official testing coverage criteria I can approve but we should discuss in the NY
Description
SelectButton
Screenshots/Recordings
Pre-merge author checklist
Pre-merge reviewer checklist