feat(Button): Icon support and default style updates #51
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
==========================================
- Coverage 68.5% 64.04% -4.47%
==========================================
Files 22 22
Lines 435 470 +35
Branches 92 104 +12
==========================================
+ Hits 298 301 +3
- Misses 107 127 +20
- Partials 30 42 +12
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## v5 #51 +/- ##
==========================================
+ Coverage 66.29% 66.88% +0.58%
==========================================
Files 22 22
Lines 451 456 +5
Branches 91 91
==========================================
+ Hits 299 305 +6
+ Misses 123 122 -1
Partials 29 29
Continue to review full report at Codecov.
|
Co-Authored-By: Mathew Morris <[email protected]>
ghost
requested review from
Co-Authored-By: Erik Shestopal <[email protected]>
This comment has been minimized.
This comment has been minimized.
src/Button/Button.js
Outdated
| css` | ||
| background-color: ${hover.backgroundColor || backgroundColor}; | ||
| border-color: ${hover.borderColor || hover.backgroundColor || backgroundColor}; | ||
| color: ${hover.fontColor || fontColor}; |
There was a problem hiding this comment.
When hovering over a text only button, I'm not seeing the text color change per the Figma specs. Same things goes for the active state.
src/theme.js
Outdated
| const fonts = [ | ||
| { | ||
| name: 'Tiempos', | ||
| url: '//cdn.heydoctor.com/fonts/TiemposHeadlineWeb-Semibold.woff2', |
There was a problem hiding this comment.
I'm not sure if we need to include this in the theme - could you elaborate on how you seeing this used in the consuming apps?
There was a problem hiding this comment.
The goal here was to default as much as possible to HeyDoctor/GoodRx design, but I can understand leaving this out for the heydoctor theme file in each project.
There was a problem hiding this comment.
@kylealwyn It was just added to provide it as a default font. If we don't want to add it to the library we can add it individually to each app.
src/theme.js
Outdated
| backgroundColor: colors.white, | ||
| fontColor: colors.primary, | ||
| borderColor: colors.primary, | ||
| textColor: colors.primary, |
There was a problem hiding this comment.
I agree with @cehsu here - fontColor and textColor will be too easy to conflate. I propose text buttons be their own variant, e.g. greyText, so we don't have to be too tricky with the text prop
Codecov Report
@@ Coverage Diff @@
## v5 #51 +/- ##
==========================================
+ Coverage 66.29% 66.88% +0.58%
==========================================
Files 22 22
Lines 451 456 +5
Branches 91 91
==========================================
+ Hits 299 305 +6
+ Misses 123 122 -1
Partials 29 29
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## v5 #51 +/- ##
==========================================
+ Coverage 66.29% 66.88% +0.58%
==========================================
Files 22 22
Lines 451 456 +5
Branches 91 91
==========================================
+ Hits 299 305 +6
+ Misses 123 122 -1
Partials 29 29
Continue to review full report at Codecov.
|
kylealwyn
changed the title
|
Echoing what Claire brough up here, are we still going to support these types of alerts? (I reached out to Jay+Mike for comment)
|
|
No, the bars were legacy and we should only support the banner style in the system |
* v5: feat(Button): Icon support and default style updates (#51)
* v5: feat(Button): Icon support and default style updates (#51) feat(Colors): Updates colors (#60) chore(release): 4.1.6 fix(FormError): Fix crash in case context is null (#59) chore(release): 4.1.5 fix: Use updater function in setState for Formbot updates (#58) chore(release): 4.1.4 fix(validations): Fix handling of async validations as well as order of onChange notification (#57) chore(release): 4.1.3 Async Formbot Validations (#56)
Update
molekuledefault stylesSummary
Several updates to align with new HeyDoctor brand including:
rollup(had an issue compiling on current version)Risk factor
Some of the color updates could introduce a breaking change so I can consider just deprecating after we evaluate in the apps.
New button page
