Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(picker): resolve font-style, focus-ring, border issues #1618

Merged
merged 10 commits into from
Feb 24, 2023

Conversation

pfulton
Copy link
Collaborator

@pfulton pfulton commented Feb 9, 2023

Description

This addresses several issues with the Picker:

  • Uses font-style: normal instead of italic for the placeholder
  • Fixes an issue where the focus-ring wasn't aligned properly in Express
  • Updates the values for a custom property which were causing both Spectrum and Express to have a border-width when Express should not have a border.
  • Updates the docs site to add classes to the "switcher" implementations of the Picker so that they'll display properly.

How and where has this been tested?

  • How this was tested:
  • Browser(s) and OS(s) this was tested with:

Screenshots

To-do list

  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have tested these changes in Windows High Contrast mode.
  • This pull request is ready to merge.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2023

🚀 Deployed on https://pr-1618--spectrum-css.netlify.app

@github-actions github-actions bot temporarily deployed to pull request February 9, 2023 22:41 Inactive
Copy link
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Some questions...

Copy link
Contributor

@lunarfusion lunarfusion left a comment

Choose a reason for hiding this comment

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

Old focus indicator used box-shadow which can now be removed (lines 190 220, 496, 509, 518, 521).

@github-actions github-actions bot temporarily deployed to pull request February 14, 2023 19:57 Inactive
@pfulton pfulton force-pushed the fix--picker-border branch 2 times, most recently from 401d213 to 561b087 Compare February 15, 2023 18:36
@github-actions github-actions bot temporarily deployed to pull request February 15, 2023 18:42 Inactive
@Westbrook
Copy link
Contributor

Westbrook commented Feb 15, 2023

image

Is this rectangle intended?
  1. Go here
  2. Click "Select a Country"
  3. Press Tab while mouse remains over "Select a Country"

This also show how the quiet Picker has visible UI in non-interactive places. Meaning you can't click the blue line or where the blue line is and get the Picker for focus, nor the rectangle implied by the Blue line.

Copy link
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

I'm happy to approve this is you'd like, but wanted to get some nits and conceptual questions in front of you before I did.

@github-actions github-actions bot temporarily deployed to pull request February 21, 2023 19:02 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 24, 2023 14:36 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 24, 2023 14:51 Inactive
@pfulton pfulton merged commit c8f7c3b into main Feb 24, 2023
@pfulton pfulton deleted the fix--picker-border branch February 24, 2023 14:55
castastrophe pushed a commit that referenced this pull request Feb 28, 2023
* fix(picker): correct focus-ring issues
* fix(picker): prefer normal instead of italic for placeholder font-style
* fix(picker): resolve focus indicator issues
* fix(picker): resolve missing quiet focus indicator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants