Skip to content

Whcm fixes#2620

Merged
devongovett merged 13 commits into
adobe:mainfrom
jnurthen:WHCM-fixes
Dec 23, 2021
Merged

Whcm fixes#2620
devongovett merged 13 commits into
adobe:mainfrom
jnurthen:WHCM-fixes

Conversation

@jnurthen
Copy link
Copy Markdown
Contributor

@jnurthen jnurthen commented Dec 1, 2021

partial resolution for #2529

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Test the following stories in WHCM

  • accordion
  • actionbar
  • avatar
  • breadcrumbs
  • tooltip
  • toast
  • tags
  • statuslight
  • sidenav
  • progressCircle
  • progessBar
  • dialog
  • tabs
Component Screenshot B4 Screenshot After
Accordion Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_50_26 PM Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_50_17 PM
ActionBar Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_52_25 PM Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_52_13 PM
Avatar Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_53_11 PM Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_53_03 PM
Breadcrumbs Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_54_06 PM Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_53_54 PM
Tooltip Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_54_25 PM Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_54_44 PM
Toast Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_55_11 PM Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_55_20 PM
Tags TagGroup - onRemove ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 5_11_36 PM TagGroup - onRemove ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 5_11_18 PM
Statuslight Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_56_04 PM (1) Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_55_50 PM
Sidenav Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_56_50 PM Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_56_42 PM
ProgressCircle Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_57_30 PM Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_57_17 PM
ProgressBar Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_58_11 PM Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_58_01 PM
Dialog Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_58_46 PM Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_58_37 PM
Tabs Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_59_39 PM Accordion - disabledKeys_ files, shared ⋅ Storybook and 1 more page - Work - Microsoft​ Edge 11_30_2021 4_59_31 PM

🧢 Your Project:

@media (forced-colors: active) {
.spectrum-Accordion-itemHeader {
&:focus-ring {
outline: 3px solid CanvasText;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How is the focus style drawn for .spectrum-Accordion-itemHeader without WHCM? Should this be?:

@media (forced-colors: active) {
  .spectrum-Accordion-itemHeader {
    &:focus-ring {
      &:after  {
        background-color: CanvasText;
      }
    }
  }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

without WHCM it is a border at the left hand side. Really too subtle for WHCM. IMO it is not enough which is why I added the outline.

@majornista
Copy link
Copy Markdown
Collaborator

Are tooltip example images reversed?

@jnurthen
Copy link
Copy Markdown
Contributor Author

jnurthen commented Dec 1, 2021

Are tooltip example images reversed?

They were. Fixed the screenshots.

Copy link
Copy Markdown
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

For the most part looks good to me, just one change really required for the Tag active styling. @snowystinger perhaps we should do the same to the breadcrumbs and tabs css that we did for the other PR? IMO we can do that in a follow up since the css files for breadcrumbs and tabs isn't as complex

@media (forced-colors: active) {
.spectrum-Accordion-itemHeader {
&:focus-ring {
outline: 3px solid CanvasText;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Stylistically looks fine to me, perhaps should be a box shadow? cc @snowystinger

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that is typically our preference, i do not remember why at this time though
but if you mix outline and box shadow you can get some odd results

}

@media (forced-colors: active) {
.spectrum-Tags-item {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like the Tags are missing a distinguishable active style in WHCM. Clicking on the Tag in the "icon, removable" story in non-WHCM displays a style change but not in WHCM

@snowystinger
Copy link
Copy Markdown
Member

@LFDanLu followup for it is fine with me now that we have an example to go off of

@devongovett devongovett merged commit 9ce5df5 into adobe:main Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants