[Theme] - Change Primary Buttons colors for Secondary Theme #651
Conversation
|
Oh I forgot to do/review the disabled comment, yeah I will take a look at it |
|
@benkolde can you review the Disabled button section and choose 1/2/3 option please. (easiest less impactful option would be 3 for me) |
|
@benkolde the border was never an issue / concern. You probably didn't noticed but in the review app secondary theme has a background of #F0EEEC (defaultLight) instead of what we have in Figma #F2F0EF (teritaryTint3) the diference is not noticeable but if we change to tint3 we are creating a visual regression for primary theme. so It is better to just use defaultLight for both |
|
@daigof ahh i see. Okay yeah, number 3 is good |
There was a problem hiding this comment.
No concerns about the actual styling but would like to see your thoughts on how to better organize 1:1 theme mapping exceptions in this repo.
| @@ -0,0 +1,31 @@ | |||
| import { ThemeColors, ThemeType } from '../../constants'; | |||
|
|
|||
| export const primaryButtonFontColor = (theme: ThemeType) => | |||
There was a problem hiding this comment.
This is great! I wonder if we should add a comment at the top of the file that explains that any ternary that is checking theme.__type should probably be moved into a util function in this file? (With the one exception being the Icon component, as you left it).
This isn't not a blocking comment--thank you for this cleanup 🙂
Taken from https://www.notion.so/curology/Audit-of-Staging-Environment-0394febd9b934132887ecf6449fedfc8
and Figma https://www.figma.com/file/FjOFrQaYVSdlaMIhpgR192/Ruth-UI?node-id=2300%3A18669&viewport=480%2C520%2C0.8739789724349976
I tested other Custom colors for
buttonColorprop and none look good for secondary theme. I dont know if design team is aware of this custom prop, I think we should explore the posibility to remove that Button prop and treat the special case in the page itself and not in Radiance the only valid usage for it I found was in dashboard modular cancellation flow, can be easily reworked. The other usages were in Admin pages which could be refactored without much design impactReview App: https://curology-radiance-pr-651.herokuapp.com/?path=/story/components-button-button--default
Disabled Button Review
@benkolde if we change disabled button to tertiaryTint3 background there is a visual change in Primary Theme
PRIMARY THEME with
tertiaryTint3 background #F8F8FAPRIMARY THEME with
defaultLight background #EDEDF0current in PRODOptions:
1- Accept the change for Primary Theme
2- Add a conditional and use different background colors for themes
3- Accept to use the current defaultLight background color and accept the change to secondary theme (barely noticeable)
SECONDARY THEME with
tertiaryTint3 background #F2F0EFSECONDARY THEME with
defaultLight background #F0EEEC