Tabs Examples: Improve high contrast support and code quality #2194
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,107 +1,71 @@ | ||
| .tabs { | ||
| font-family: "lucida grande", sans-serif; | ||
| } | ||
|
|
||
| [role="tablist"] { | ||
| min-width: 550px; | ||
|
There was a problem hiding this comment. Why are we setting a min-width? |
||
| } | ||
|
|
||
| [role="tab"], | ||
| [role="tab"]:focus, | ||
| [role="tab"]:hover { | ||
| position: relative; | ||
| z-index: 2; | ||
| top: 2px; | ||
| margin: 0; | ||
| margin-top: 4px; | ||
| padding: 3px 3px 4px; | ||
jongund marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| border: 1px solid hsl(219deg 1% 72%); | ||
| border-bottom: 2px solid hsl(219deg 1% 72%); | ||
| border-radius: 5px 5px 0 0; | ||
| overflow: visible; | ||
| background: hsl(220deg 20% 94%); | ||
| outline: none; | ||
|
There was a problem hiding this comment. We should probably set a transparent outline. There was a problem hiding this comment. The updated examples are actively styling keyboard focus, so it will be visually confusing if outline also appears. We do not want two focus rings. |
||
| font-weight: bold; | ||
| } | ||
|
|
||
| [role="tab"][aria-selected="true"] { | ||
| padding: 2px 2px 4px; | ||
| margin-top: 0; | ||
| border-width: 2px; | ||
| border-top-width: 6px; | ||
| border-top-color: rgb(36 116 214); | ||
| border-bottom-color: hsl(220deg 43% 99%); | ||
|
There was a problem hiding this comment. I don't understand any of these CSS changes? They don't seem to relate to the requested changes? There was a problem hiding this comment. To support high contrast modes I am trying to emphasize at least 2 pixel spacing and borders to support operating system high contrast modes for focus styling. This is easier for developers to see if pixels are used in the code. There was a problem hiding this comment. Ah I see what you’re saying now. But wouldn’t a transparent outline achieve the same for high contrast mode? Would’ve been a far less invasive change. |
||
| background: hsl(220deg 43% 99%); | ||
| } | ||
|
|
||
jongund marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| [role="tab"][aria-selected="false"] { | ||
| border-bottom: 1px solid hsl(219deg 1% 72%); | ||
| } | ||
|
|
||
| [role="tab"] span.focus { | ||
| display: inline-block; | ||
| margin: 2px; | ||
| padding: 4px 6px; | ||
| } | ||
|
|
||
| [role="tab"]:hover span.focus, | ||
| [role="tab"]:focus span.focus, | ||
| [role="tab"]:active span.focus { | ||
| padding: 2px 4px; | ||
| border: 2px solid rgb(36 116 214); | ||
| border-radius: 3px; | ||
| } | ||
|
|
||
| [role="tabpanel"] { | ||
| padding: 5px; | ||
| border: 2px solid hsl(219deg 1% 72%); | ||
| border-radius: 0 5px 5px; | ||
| background: hsl(220deg 43% 99%); | ||
| min-height: 10em; | ||
| min-width: 550px; | ||
| overflow: auto; | ||
|
There was a problem hiding this comment. Same here, I don't understand why these changes are needed. Can you elaborate? There was a problem hiding this comment. The visual rendering is not good in windows less than 550 pixels. There was a problem hiding this comment. Is this related to the content change? Can you elaborate? |
||
| } | ||
|
|
||
| [role="tabpanel"].is-hidden { | ||
| display: none; | ||
| } | ||
|
|
||
| [role="tabpanel"] p { | ||
| margin: 0; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot clearer, nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two main issues:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two main issues:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also update the carousel example to remove all references to the TV shows?