Skip to content

Conversation

@frabbit
Copy link
Contributor

@frabbit frabbit commented Aug 27, 2019

No description provided.

@frabbit frabbit changed the base branch from master to feat_design-system-2 August 27, 2019 09:52
@frabbit
Copy link
Contributor Author

frabbit commented Aug 27, 2019

Please note that this PR is not against master, it's against a design system 2 branch.

@jkettmann
Copy link
Contributor

jkettmann commented Aug 29, 2019

I checked the styleguide and all seems to look good. I know we talked about this already, but I'm still not happy with the focus outline.

I had another look and found this resource (honestly it was the first result on google). They say that you shouldn't remove outline because keyboard users won't have visual feedback about which element is currently focused.

An alternative they mention is styling the element itself. So if you give a button a different border or shadow, you should be fine.

I had a look at Airbnb. Their important buttons are styled this way:

.button:focus {
  box-shadow: rgb(255, 255, 255) 0px 0px 0px 4px, rgb(113, 113, 113) 0px 0px 0px 5px, rgba(255, 255, 255, 0.5) 0px 0px 0px 6px !important;
  outline: none !important;
  transition: box-shadow 0.2s ease 0s !important;
}

Here are screenshots of our and their buttons

v2-button-outline

v2-button-outline-comparison-airbnb

What do you think? We can, of course, move the discussion somewhere else :-) Apart from this issue I'm happy with the PR

@frabbit
Copy link
Contributor Author

frabbit commented Aug 29, 2019

@jkettmann i didn't removed the outline in this pr (should be the default). The airbnb solution works but depends on the exact color of the background. Not sure if we want it that way...

image

@jkettmann
Copy link
Contributor

Ok, you're right. We discussed that anyways already.

@frabbit frabbit requested review from drinchev and jkettmann August 30, 2019 16:16
Copy link
Contributor

@drinchev drinchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome btw let's also fix the theme part ( maybe in another commit :) )

@frabbit frabbit merged commit 78a54f9 into feat_design-system-2 Sep 2, 2019
@frabbit frabbit deleted the feat_new-buttons branch September 2, 2019 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants