primer / css

Primer is GitHub's design system. This is the CSS implementation
https://primer.style/css
MIT License
12.55k stars 1.21k forks source link

Button styling is slightly and inconsistently broken #1341

Closed laserlemon closed 2 years ago

laserlemon commented 3 years ago

I put together a test page that uses Primer view components to render several variations on the Primer::ButtonComponent, using all three supported tags (button, a, and summary) as well as both disabled and enabled states. For button tags, I used the disabled attribute and for a/summary tags, I used aria-disabled.

From what I can tell, there are five issues:

  1. Disabled, default scheme buttons (for all tags) show a slight shadow as if the button is pushable. This shadow is absent from outline and danger scheme buttons.
  2. Disabled, outline scheme buttons (for button and a tags) change the icon color on hover. The summary tag button does not. I'm not sure what the intended behavior here is but there's at least a consistency problem.
  3. Disabled, danger scheme buttons (for button and a tags) change the icon color on hover. The summary tag button does not. There's the same consistency problem described in number 3 and the additional problem of the icon color changing to white on hover which has sort of a disappearing effect.
  4. Disabled, primary scheme buttons (for button and a tags) show a slight shadow as if the button is pushable. This shadow is (correctly) absent from outline and danger scheme buttons.
  5. Disabled, primary scheme buttons (for all tags) show a gray icon, which looks out of place. Should the icon preserve the white styling of the button text?

Numbers 2, 3, and 5 are also problematic in dark and dimmed themes. Demo included for the dimmed theme.

Demos

Light theme, hovers

2021-04-13 10 24 49

Light theme, clicks

2021-04-13 10 28 45

Dimmed theme, hovers and clicks

2021-04-13 10 49 44

Context

I also experience these same behaviors on Firefox 87.0 and Chrome 89.0.4389.114.

simurai commented 3 years ago

The pressed (:active) state of btn-primary seems to have a gray border in "Dark high contrast":

button

Also @Juliusschaeper noticed that the pressed (:active) state of btn-primary has a gray background, but only in Safari. I guess Chrome also uses the :focus style so it is still green? 🤔

6AC688DF-FC18-410F-9283-EB268170F695_1_105_c

mperrotti commented 3 years ago

@simurai - I was looking at the issues you posted about while working on https://github.com/github/primer/issues/263

~I can't reproduce the Safari issue, but the gray border seems to be happening on all dark modes. It looks like we just forgot to specify colors.btn.primary.activeBorder:~ ~https://github.com/primer/primitives/blob/main/data/colors_v2/vars/component_dark.ts#L78~

~I can submit a PR to fix that as part of my work on that issue.~

Edit: Instead of adding a activeBorder property to colors.btn.primary, I'm just going to set the border-color in Primer CSS.

yaili commented 2 years ago

I transferred this issue to https://github.com/github/primer/issues/468 - closing as duplicate.