Skip to content

Whcm buttons#2626

Merged
devongovett merged 10 commits into
adobe:mainfrom
jnurthen:WHCM---buttons
Dec 22, 2021
Merged

Whcm buttons#2626
devongovett merged 10 commits into
adobe:mainfrom
jnurthen:WHCM---buttons

Conversation

@jnurthen
Copy link
Copy Markdown
Contributor

@jnurthen jnurthen commented Dec 2, 2021

Partially resolves #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:

Screenshots

Button Before

Screen Shot 2021-12-01 at 4 48 18 PM

### Button After

Screen Shot 2021-12-01 at 4 46 47 PM

### ToggleButton Before

Screen Shot 2021-12-01 at 4 48 40 PM

### ToggleButton After

Screen Shot 2021-12-01 at 4 47 30 PM

### LogicButton Before

Screen Shot 2021-12-01 at 4 49 04 PM

### LogicButton After

Screen Shot 2021-12-01 at 4 49 29 PM

🧢 Your Project:

@LFDanLu
Copy link
Copy Markdown
Member

LFDanLu commented Dec 16, 2021

@jnurthen my XD is broken even after a reinstall, I'll try and debug it soon. In the meantime can you confirm that a unfocused CTA button should have the same color fill as the left color picker box for "Button Text" in the Windows High Contrast setting? The screenshot you attached shows the CTA button while focused.

This is mine
CTA WHCM

EDIT: Nvm, got my XD working again after a restart

.spectrum-ActionButton {
&:focus-ring,
&.is-focused {
outline: CanvasText solid 3px;
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.

The focus ring for action buttons seems a bit thick compared to the XD files
Local:
png2

XD:
image

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.

Note: Made a change to remove this here: b3d5e33, will need to ask Spectrum if it should be a halo focus ring or not

From XD (emphasized and selected):
image

From local:
fafawfawf

Copy link
Copy Markdown
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

lgtm, worked with @LFDanLu I can't currently test on a windows machine, but this is what I expect

@devongovett devongovett merged commit d4669af into adobe:main Dec 22, 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.

Support for Windows High Contrast Themes

4 participants