-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix(Button): redesign of existing Button component #3473
Conversation
…tton in ButtonGroup components.
Updated various uses of the Button component. Uses of Button component that did NOT match the new design were changed to LegacyButton. LegacyButton uses will be changed/addressed in future redesigns. Updated the Button Storybook.
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for ohif-platform-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Codecov Report
@@ Coverage Diff @@
## master #3473 +/- ##
=======================================
Coverage 38.27% 38.27%
=======================================
Files 82 82
Lines 1348 1348
Branches 303 303
=======================================
Hits 516 516
Misses 666 666
Partials 166 166 Continue to review full report in Codecov by Sentry.
|
extensions/measurement-tracking/src/panels/PanelMeasurementTableTracking/ActionButtons.tsx
Outdated
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.
Looks great, see my comments and maybe update the PR before the first image to explain that this is done to reduce the complexity of variants and colors and etc etc, and maybe just mention the 4 different syntaxes they can use now (instead of who knows how many before)
Added TODOs for every LegacyButton reference. Updated the look-and-feel for the viewport notification banner/dialogue. Added a minimum width for text-only buttons. Those with icons do not get a minimum width.
Thanks. I updated the PR overview. |
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.
Looks great, thanks for taking care of this
Context
New Button component designs thanks to @dan-rukas (see the screen shots below). The redesign improves various items:
A consistent look-and-feel with the rest of OHIF with better button styles, sizes and colours
A reduction in complexity for developers using the
Button
. The previousButton
component allowed for many (too many) variant, colour and border permutations, making for a confusing developer experience. Furthermore, with so many permutations, maintaining a consistent button look-and-feel proved to be very difficult. Now there are four basic permutations:boolean
attribute of theButton
like so...Changes & Results
Button
component to match the new styles.Button
component.Button
component that did NOT match the new design were changed toLegacyButton
.LegacyButton
uses will be changed/addressed in future redesigns.Button
Storybook.Testing
All the various uses of
Button
should reflect the new design. Those that do NOT include...ButtonGroup
buttonsChecklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment