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

Add textColor prop to <Button /> to allow overriding of colors #545

Merged
merged 3 commits into from Jun 24, 2019

Conversation

pfarejowicz
Copy link
Contributor

This change is inspired by the limitations of the gestalt Button with different colored backgrounds. Previously, the white button worked well against a darkGray background as it matched in color.
image
However, with user education now using the Pinterest blue and error states using the Pinterest red, this no longer scales to those backgrounds
image

With this change, the user of Button can pass an optional textColor prop to override that font color and make the text color match against the background of the container parent.
image

This matches 1:1 with what the PDS spec says user education and error states is supposed to be, but we previously never followed this guideline.
image
Before this change, developers would have to hack together custom buttons to get blue text but with this new prop, it will be included in gestalt and we can remove those hacks from the codebase.

@pfarejowicz pfarejowicz requested a review from a team as a code owner June 23, 2019 22:59
@btraut
Copy link
Contributor

btraut commented Jun 24, 2019

How restrictive do you think we should be? This change allows users to create red buttons with red text (and a few other illegal states). Instead, we could just create color schemes that encompass both button and text color. I'm not saying at all that this is the right approach, but I wanted to make sure you considered it.

@pfarejowicz
Copy link
Contributor Author

How restrictive do you think we should be? This change allows users to create red buttons with red text (and a few other illegal states). Instead, we could just create color schemes that encompass both button and text color. I'm not saying at all that this is the right approach, but I wanted to make sure you considered it.

Yeah I considered that possibility of abuse, my mindset was designers and developers here at Pinterest are not going to go off-spec and create color combinations like that so I gave them the trust to control their own button colors and text colors with the hope they only use it for special cases like white - while leaving the option for the open source world to do whatever. The alternative would probably be extending color from what it is now blue | red | white | ... to add white-red and white-blue to the list so its more limiting. Is that something you'd advise, I'm open to both solutions?

@pfarejowicz pfarejowicz merged commit 9a4c820 into pinterest:master Jun 24, 2019
@pfarejowicz pfarejowicz deleted the add_button_text_color branch June 24, 2019 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants