feat(sp-picker): s2 theme update - no borders for picker #5733
Conversation
🦋 Changeset detectedLatest commit: 55e8216 The changes in this PR will be included in the next version bump. This PR includes changesets to release 84 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
.circleci/config.yml
Outdated
| current_golden_images_hash: | ||
| type: string | ||
| default: 5cb7586aae24a30d566d452d4e0b3c934fe3b9f0 | ||
| default: 545766d1fd28704c1920e13b3fb8bf24f9c7dbba |
There was a problem hiding this comment.
We usually update the hash once the PR is approved to make sure we are not overriding any unintended visual changes to main
There was a problem hiding this comment.
Did not know, sorry! I reverted it right now, thank you!
…the code review request
| --system-picker-border-color-key-focus: transparent; | ||
| --system-picker-border-color-disabled: transparent; | ||
| --system-picker-border-width: 0px; | ||
| --system-picker-border-width: var(--spectrum-border-width-100); |
There was a problem hiding this comment.
I was also noticing that the label text was positioned further up within the picker before, so reverting back to border-width-100 also solves that issue!
But I'm wondering if this border-width is desired for the invalid variant, or do we want to adjust maybe to border-width-200?
There was a problem hiding this comment.
@iuliauta definitely knows best, but the border-width-200 is in the specs for picker in S2. This suggestion makes sense to me!
There was a problem hiding this comment.
Yes, it seems that I should use --system-picker-border-width: var(--spectrum-border-width-200); for s2. I will update it, thank you for all the details, I was not aware of the label positioning, but it is good information for me.
There was a problem hiding this comment.
Thanks @iuliauta, this looks great!
One thing I want to call out that I think is out of scope for this work, is that there's a diff in the VRTs for the position of the text (just for L, it looks like), where there's more spacing above the text where we increased the border width, so it sits lower within the Picker.
I'm wondering if part of the issue is that we're not calculating the border-width out of the spacing like we do with button, for instance doing something like:
--spectrum-picker-spacing-edge-to-text: calc(var(--spectrum-component-edge-to-text-100) - var(--spectrum-picker-border-width));
This would help keep the spacing consistent even if the border width changes.
All of that said though, since the fix would need to happen outside of the theme files, I think it's out of scope, but I wanted to explain what I think might be happening here.
Thank you for pointing it out! I tried to see how this change will influence the themes and it seems that it impacts spectrum-1 theme and I suggest to address this improvement in another PR, unrelated to CSS themes PRs (as you already mentioned in your message). |
rise-erpelding
added
ready-for-merge
Description
Picker border color should be hidden in S2 theme
Motivation and context
Fix the regressions found after changing from the express theme to the spectrum-two theme.
Related issue(s)
Screenshots (if appropriate)
Spectrum 2
Spectrum 1
Express
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Descriptive Test Statement
Device review