fix(picker): quiet side label picker positioning #2465

jenndiaz · 2024-01-25T22:53:09Z

Description

Addresses concerned raised in adobe/spectrum-web-components#2963 where the quiet picker and it's associated side label does not align with text of other components, making it appear incorrect when used with other components.

For before look at #2231

Changes:

FieldLabel:

use --spectrum-field-label-top-margin-* tokens and margin to adjust position of side label to match design specs

Picker

Remove negative top margin from the quiet side label
Use correct tokens for picker side label margins
add displaying with the switch component to controls for picker storybook

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

On the docs site for picker and field label

side field labels match design specs (side label positioning is slightly lower, 4px for medium, than previously)

Storybook for picker

controls should all work as expected, particularly looking at quiet with a side label and a switch at various t-shirt sizes

Chromatic

displays kitchen sink grouping of picker which includes, a standard, standard quiet, loading, invalid, keyboard focused, standard with side label and switch, and quiet with side field label and switch

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

T-shirt sizing with other components

Storybook Kitchen Sink

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
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.
✨ This pull request is ready to merge. ✨

jenndiaz · 2024-01-25T22:54:25Z

components/fieldlabel/index.css

@@ -27,7 +27,7 @@ governing permissions and limitations under the License.
  --spectrum-fieldlabel-bottom-to-text: var(--spectrum-component-bottom-to-text-75);
  --spectrum-fieldlabel-font-size: var(--spectrum-font-size-75);

-  --spectrum-fieldlabel-side-padding-top: var(--spectrum-component-top-to-text-75);
+  --spectrum-fieldlabel-side-margin-block-start: var(--spectrum-field-label-top-margin-small);


This was previously using the wrong token

jenndiaz · 2024-01-25T23:05:15Z

components/picker/index.css

@@ -449,7 +449,7 @@ governing permissions and limitations under the License.
  background-color: var(--highcontrast-picker-background-color, transparent);

  &.spectrum-Picker--sideLabel {
-    margin-block-start: calc(-1 * var(--spectrum-picker-spacing-top-to-text-side-label-quiet));
+    margin-block-start: 0;


negative margin is only needed for quiet variants when the field label position is top

github-actions · 2024-01-25T23:38:24Z

File metrics

Summary

Total size: 3.94 MB^*
Total change (Δ): ⬆ 0.02 KB (0.00%)
Table reports on changes to a package's main file. Other changes can be found in the collapsed "Details" below.

Package	Size	Δ
fieldlabel	6.65 KB	⬆ 0.09 KB
picker	30.82 KB	⬇ 0.09 KB

Details

fieldlabel

File	Head	Base	Δ
index-base.css	6.65 KB	6.56 KB	⬆ 0.09 KB (1.35%)
index-vars.css	6.65 KB	6.56 KB	⬆ 0.09 KB (1.35%)
index.css	6.65 KB	6.56 KB	⬆ 0.09 KB (1.35%)
mods.json	0.80 KB	0.80 KB	⬆ 0.01 KB (0.88%)

picker

File	Head	Base	Δ
index-base.css	28.94 KB	29.02 KB	⬇ 0.09 KB (-0.29%)
index-theme.css	2.46 KB	2.46 KB	-
index-vars.css	30.82 KB	30.91 KB	⬇ 0.09 KB (-0.27%)
index.css	30.82 KB	30.91 KB	⬇ 0.09 KB (-0.27%)
mods.json	2.81 KB	2.81 KB	-
themes/express.css	1.47 KB	1.47 KB	-
themes/spectrum.css	1.58 KB	1.58 KB	-

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

github-actions · 2024-01-25T23:38:38Z

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

mdt2

This is looking good to me. Just a couple nit picky comments/suggestions

mdt2 · 2024-01-29T19:16:38Z

components/picker/stories/template.js

+		${withSwitch ?
+			Switch({
+				...globals,
+				size,
+				label: "Toggle switch",
+				customStyles: {
+					"padding-inline-start": "15px"
+				}
+			})
+			: null}


I wonder if we could use lit's when here instead of a ternary 🤔 Would look something like:

${when(withSwitch, () => Switch({ ...globals, size, label: "Toggle switch", customStyles: { "padding-inline-start": "15px" } }) }

mdt2 · 2024-01-29T19:29:10Z

components/picker/stories/picker.stories.js

@@ -35,6 +37,15 @@ export default {
 			options: ["top", "left"],
 			control: { type: "select" },
 		},
+		withSwitch: {
+			name: "Display with a switch component",


Do we want this to show in the control table, or is this just intended for Chromatic? It looks a bit odd when the picker label is on the top. We could also maybe make this control only show in the table when picker label is "left." Just throwing ideas out there :)

thanks for catching this, it does look wrong when the field label is top, but I do still think there is value in being able to toggle this control for the side label. Adjusted the control accordingly.

mdt2 · 2024-02-01T17:00:17Z

One other thing I just thought of: can the quiet picker label wrap? Have we accounted for that?

pfulton

Thanks!

mdt2

Thanks for making those changes, LGTM!

castastrophe

Can you update the stories with the new syntax? Otherwise this looks great!

castastrophe · 2024-02-08T16:35:29Z

components/picker/stories/picker.stories.js

@@ -1,4 +1,6 @@
 import { Default as MenuStories } from "@spectrum-css/menu/stories/menu.stories.js";
+import { html } from "lit";
+import isChromatic from "chromatic/isChromatic";


Can you update this with the new window.isChromatic() syntax now that that tool is merged?

castastrophe · 2024-02-08T16:36:59Z

components/picker/stories/picker.stories.js

-		() => MenuStories(MenuStories.args)
-	],
-};
-
 export const WithForcedColors = Template.bind({


Can you update the ForcedColors run to use the Group as well?

jenndiaz commented Jan 25, 2024

View reviewed changes

jenndiaz force-pushed the jenndiaz/fieldlabel-and-picker branch from 5c1e187 to 686b75e Compare January 25, 2024 23:00

jenndiaz commented Jan 25, 2024

View reviewed changes

jenndiaz force-pushed the jenndiaz/fieldlabel-and-picker branch from 686b75e to 7c1526f Compare January 25, 2024 23:24

jenndiaz force-pushed the jenndiaz/fieldlabel-and-picker branch from 62e0e4a to 57cf5fe Compare January 25, 2024 23:42

jenndiaz added the run_vrt For use on PRs looking to kick off VRT label Jan 25, 2024

jenndiaz marked this pull request as ready for review January 26, 2024 22:17

jenndiaz requested review from pfulton, jawinn and mdt2 January 26, 2024 22:28

jenndiaz added the ready-for-review label Jan 29, 2024

mdt2 reviewed Jan 29, 2024

View reviewed changes

jenndiaz removed the run_vrt For use on PRs looking to kick off VRT label Feb 1, 2024

jenndiaz requested a review from mdt2 February 5, 2024 15:42

pfulton approved these changes Feb 6, 2024

View reviewed changes

mdt2 approved these changes Feb 6, 2024

View reviewed changes

jenndiaz added the pending-swc-validation label Feb 7, 2024

pfulton force-pushed the jenndiaz/fieldlabel-and-picker branch from 633ff12 to 2684376 Compare February 7, 2024 19:28

jenndiaz added run_vrt For use on PRs looking to kick off VRT and removed pending-swc-validation labels Feb 7, 2024

castastrophe requested changes Feb 8, 2024

View reviewed changes

jenndiaz removed the run_vrt For use on PRs looking to kick off VRT label Feb 8, 2024

jenndiaz force-pushed the jenndiaz/fieldlabel-and-picker branch from 65adf1c to 952d1df Compare February 8, 2024 22:46

jenndiaz requested a review from castastrophe February 9, 2024 15:54

jenndiaz added 3 commits February 12, 2024 15:24

fix(fieldlabel): use margin top tokens

f03a8ce

fix(picker): adjust sidelabel position

1be3495

docs(fieldlabel): mods

8ae8edd

jenndiaz added 6 commits February 12, 2024 15:24

docs(picker): adds switch to sb controls

5ca3dfe

docs(picker): group stories for chromatic

9f07c25

refactor(picker): updates storybook

ef323cf

docs(picker): add wrapping side label to sb

875d399

docs(picker): update storybook post VRT

13101a0

docs(picker): update ischromatic syntax

b4a7526

pfulton force-pushed the jenndiaz/fieldlabel-and-picker branch from 43eee15 to b4a7526 Compare February 12, 2024 20:24

pfulton merged commit 537f0b8 into main Feb 12, 2024
10 of 11 checks passed

pfulton deleted the jenndiaz/fieldlabel-and-picker branch February 12, 2024 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(picker): quiet side label picker positioning #2465

fix(picker): quiet side label picker positioning #2465

jenndiaz commented Jan 25, 2024 •

edited

Loading

jenndiaz Jan 25, 2024

jenndiaz Jan 25, 2024

github-actions bot commented Jan 25, 2024 •

edited

Loading

fieldlabel

picker

github-actions bot commented Jan 25, 2024 •

edited

Loading

mdt2 left a comment

mdt2 Jan 29, 2024 •

edited

Loading

mdt2 Jan 29, 2024

jenndiaz Feb 1, 2024

mdt2 commented Feb 1, 2024

pfulton left a comment

mdt2 left a comment

castastrophe left a comment

castastrophe Feb 8, 2024

castastrophe Feb 8, 2024

fix(picker): quiet side label picker positioning #2465

fix(picker): quiet side label picker positioning #2465

Conversation

jenndiaz commented Jan 25, 2024 • edited Loading

Description

Changes:

How and where has this been tested?

Validation steps

Regression testing

Screenshots

T-shirt sizing with other components

Storybook Kitchen Sink

To-do list

jenndiaz Jan 25, 2024

Choose a reason for hiding this comment

jenndiaz Jan 25, 2024

Choose a reason for hiding this comment

github-actions bot commented Jan 25, 2024 • edited Loading

File metrics

Summary

fieldlabel

picker

github-actions bot commented Jan 25, 2024 • edited Loading

mdt2 left a comment

Choose a reason for hiding this comment

mdt2 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

mdt2 Jan 29, 2024

Choose a reason for hiding this comment

jenndiaz Feb 1, 2024

Choose a reason for hiding this comment

mdt2 commented Feb 1, 2024

pfulton left a comment

Choose a reason for hiding this comment

mdt2 left a comment

Choose a reason for hiding this comment

castastrophe left a comment

Choose a reason for hiding this comment

castastrophe Feb 8, 2024

Choose a reason for hiding this comment

castastrophe Feb 8, 2024

Choose a reason for hiding this comment

jenndiaz commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

mdt2 Jan 29, 2024 •

edited

Loading