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

fix: remove the underline from button states #147

Merged
merged 1 commit into from Oct 23, 2019

Conversation

@SiTaggart
Copy link
Collaborator

SiTaggart commented Oct 22, 2019

Because we have now fixed the contrast of the button hover and focus states with the outline color, we can now remove the text underline.

Fixes #54

@now

This comment has been minimized.

Copy link

now bot commented Oct 22, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/twilio-dsys/paste/ktb4p9b72
🌍 Preview: https://paste-git-feat-remove-button-underline.twilio-dsys.now.sh

@richbachman

This comment has been minimized.

Copy link
Collaborator

richbachman commented Oct 22, 2019

It looks like that change killed the hover underline on the reset variant: https://paste-git-feat-remove-button-underline.twilio-dsys.now.sh/components/button#examples

Original here for comparison: https://paste.twilio.design/components/button#examples

@SiTaggart

This comment has been minimized.

Copy link
Collaborator Author

SiTaggart commented Oct 22, 2019

It looks like that change killed the hover underline on the reset variant:

So, this was by accident, but it does raise the question of; should a reset button have any styles like underlines on hover? @TheSisb @two24studios @serifluous @richbachman

@serifluous

This comment has been minimized.

Copy link
Contributor

serifluous commented Oct 22, 2019

I keep asking @TheSisb again and again about what reset is for, and I keep forgetting. Can someone remind me? 😬

As a stab in the dark, I'm going to say it should still keep the underline on hover.

@TheSisb

This comment has been minimized.

Copy link
Collaborator

TheSisb commented Oct 23, 2019

The Button component has areset for size and a reset for variant.

The variant reset simply removes all the default button styles. We added this in the first pass of the component but I'm no longer convinced we should have it. I think it's safe to remove it knowing what we now know.

The size reset omits adding the Button font-size, line-height, border-radius, font-weight, and padding that other sizes set. This was useful for buttons as links, where the additional padding seemed off.


Reflecting on those decisions and per our discussions, I think we should change our variants into a tuple shaped as level_look.

Example:

- primary_filled
- primary_outlined
- primary_link
- destructive_filled
- destructive_outlines
- destructive_link

This would allow us to add new variants with relative ease while really nailing down what the use-case is in the variant labeling. I think this change also allows us to remove reset entirely because it scopes it out.

@TheSisb

This comment has been minimized.

Copy link
Collaborator

TheSisb commented Oct 23, 2019

I approved the PR because this change is fine. We should follow it up with some additional ticketed Button work.

@richbachman

This comment has been minimized.

Copy link
Collaborator

richbachman commented Oct 23, 2019

@TheSisb thanks for the explanation and background.

@SiTaggart SiTaggart force-pushed the feat/remove-button-underline branch from c8d179d to b0af74f Oct 23, 2019
@SiTaggart SiTaggart merged commit 70d4c15 into master Oct 23, 2019
8 checks passed
8 checks passed
Semantic Pull Request ready to be squashed
Details
ci/circleci: applitools Your tests passed on CircleCI!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: prettier Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
now Deployment has completed
Details
scm/applitools No baseline conflicts found! (0 changes found)
Details
tests/applitools No visual tests ran, see "Details" for help
Details
@SiTaggart SiTaggart deleted the feat/remove-button-underline branch Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.