fix(button): support for wrapping text #2248

jawinn · 2023-10-31T19:58:07Z

Description

This update allows text wrapping within the Button component and updates the alignment of the label and icon when wrapping to conform with the design specs.

Fixes a previously introduced bug that disabled wrapping of lengthy text inside a Button. This removes the white-space property that prevented wrapping.
BREAKING CHANGE: Updates styles so that buttons that contain a workflow icon have the correct icon and text alignment. Previously everything was vertically aligned center and the text was aligned center, which did not match the design in the case of wrapping text. See screenshots for reference to the design.

This is a significant change as the vertical flex alignment of the icon is now to the top/start instead of the center, and the design defined --spectrum-component-top-to-workflow-icon-* tokens are used for the spacing between the icon and the top of the component. While button is intended to be used with workflow icons, there were not previously any guidelines around not using UI icons and even SplitButton was using one---in order to support vertical alignment of any existing usage of UI icons, some additional CSS calculations were included to add some margin based on the difference between the height of the intended workflow icon size and the actual icon size used (now accessible through --spectrum-icon-block-size since the 6.0 core tokens migration of Icon).
Adds a "Wrapping" example to Storybook. This also has a Chromatic-only version with additional variations displayed.

CSS-621

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

Verify changes with design team [@jawinn]
Button wrapping example exists in Storybook.
Button without icon wraps and has centered text (like guidelines).
Button with icon has icon aligned to the top of the component and text aligned to the left/start of the component. Matching "Design spec for wrapping behavior with icon" in screenshots.
Wrapping behavior looks correct in right-to-left text direction.
Non-wrapping examples of the button with an icon have no visual changes.

Regression testing

Validate:

A legacy documentation page (such as accordion), including:

The page renders correctly
The page is accessible
The page is responsive

A migrated documentation page (such as action group), including:

The page renders correctly
The page is accessible
The page is responsive

Screenshots

Updated wrapping behavior (Storybook):

Chromatic view of Wrapping story that includes showing backwards compatible alignment of differently sized UI icons

Reference: design spec for wrapping behavior with icon

Previous behavior:

Reference: design spec for wrapping behavior without icon

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 documentation, I have updated the documentation accordingly.
VRTs have been run and reviewed.
✨ This pull request is ready to merge. ✨

github-actions · 2023-10-31T20:05:36Z

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

jenndiaz

One comment about the static color variant background colors.

I looked at it on Safari, Firefox, and WHCM and didn't see any other issues.

components/button/stories/button.stories.js

mdt2

Looking good! +1 to Jenn's feedback and then I think this should be good

components/button/index.css

components/button/stories/button.stories.js

pfulton

Thanks for taking care of this one!

github-actions · 2023-11-10T23:27:09Z

File metrics

Summary

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

Package	Size	Δ
button	82.35 KB	⬆ 1.42 KB

Details

button

File	Head	Base	Δ
index-base.css	52.40 KB	50.98 KB	⬆ 1.42 KB (2.78%)
index-theme.css	30.53 KB	30.53 KB	-
index-vars.css	82.35 KB	80.93 KB	⬆ 1.42 KB (1.75%)
index.css	82.35 KB	80.93 KB	⬆ 1.42 KB (1.75%)
mods.json	1.77 KB	1.63 KB	⬆ 0.15 KB (8.89%)
themes/express.css	29.33 KB	29.33 KB	-
themes/spectrum.css	29.71 KB	29.71 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.

castastrophe

A couple notes but looking great!

components/button/index.css

castastrophe · 2023-11-14T15:03:00Z

components/button/index.css

+.spectrum-Button--staticWhite {
+  --spectrum-button-focus-indicator-color: var(--mod-static-black-focus-indicator-color, var(--spectrum-static-black-focus-indicator-color));
+}
+
+.spectrum-Button--staticBlack {
+  --spectrum-button-focus-indicator-color: var(--mod-static-black-focus-indicator-color, var(--spectrum-static-black-focus-indicator-color));
+}


Do these 2 need increased specificity by incorporating the .spectrum-Button selector as well?

I don't think so, unless there's a scenario you're thinking of? I just moved these up a tiny bit above the high contrast styles. They are still at the end of the file and override the same specificity of their definitions in .spectrum-Button.

We can't rely on order for apply styles appropriately because a lot of processing moves things around. Just raising the question because I've seen a few components where insufficient specificity was causing styles not to be applied after processing.

components/button/stories/button.stories.js

jawinn · 2023-11-21T16:55:09Z

This was marked as do-not-merge until the issue with UI Icons vs Workflow icons can be addressed. Per discussions, design intends Workflow icons to be used with Button, but without documentation/guidelines in place for that, it's guaranteed that buttons are currently in use that use UI icons. Including one of the demoed pagination examples using Splitbutton with the chevron, that was caught in the VRTs.

Storybook currently allows selecting any icon for Button, and the following examples show the difference in the vertical alignment of the icon. This results from the token for spacing above the icon accounting for a fixed Workflow icon size, whereas UI Icons are various sizes:

Once Icon is migrated to core tokens (in progress) and has a custom property for its size, this can be addressed to support both icon types with a CSS calc and/or min/max.

jawinn · 2024-02-08T19:21:18Z

This is ready for re-review. Storybook is updated and has a Chromatic-only template. I've updated the styles to support existing use of UI icons instead of Workflow icons to address my last comment, and have added some more explanation in the PR description.

I'm wondering if the alignment change should be marked as a breaking change. Or if it would be better to save that part of the change for Spectrum 2 ( @pfulton )?

pfulton · 2024-02-14T17:59:34Z

This is ready for re-review. Storybook is updated and has a Chromatic-only template. I've updated the styles to support existing use of UI icons instead of Workflow icons to address my last comment, and have added some more explanation in the PR description.

I'm wondering if the alignment change should be marked as a breaking change. Or if it would be better to save that part of the change for Spectrum 2 ( @pfulton )?

I'm inclined to make it a breaking change and introduce it now.

castastrophe · 2024-02-16T16:25:19Z

components/button/package.json

@@ -15,7 +15,7 @@
  },
  "main": "dist/index-vars.css",
  "peerDependencies": {
-    "@spectrum-css/icon": ">=4",
+    "@spectrum-css/icon": ">=6",


🥳 Thanks for capturing this requirement update!

castastrophe · 2024-02-16T16:27:48Z

components/button/stories/button.stories.js

+		},
+		layout: {
+		    name: "Layout",
+		    description: "How the buttons align in the preview (Storybook only).",


Just noting here that this will probably no longer be necessary after the storybook layout decorator merges - plus side, I might steal this approach for that PR #2420!

Remove the CSS that prevents the text in the button from wrapping. The guidelines for the component say "When the button text is too long for the horizontal space available, it wraps to form another line."

Add "Wrapping" story that demonstrates a button with long text that wraps to the next line. Some template adjustments and additional args added to allow for this.

BREAKING CHANGE: changes vertical flex alignment to start. Previously when the button had an icon and the text was wrapping, the icon was vertically aligned center and the text was aligned center. This fixes this to match with the design spec: for the version of the button that uses a workflow icon, the icon should stay aligned to the top, and the text should be aligned left (start). This uses the defined token for the space between the top of the component and the workflow icon (--spectrum-component-top-to-workflow-icon-*).

Rename Storybook control for stacking buttons to "layout" with specified options, which is a more appropriate name. The control has also been made visible.

Support any existing use of ui icons with the updated wrapping behavior. And add Chromatic only testing of them to the Wrapping story. Workflow icons are intended, with the use of the spectrum-component-top-to-workflow-icon tokens, but UI icons have not yet been specifically excluded in guidelines and are currently in use within SplitButton in this library. This keeps UI icons that are smaller than the intended workflow icon, better vertically centered with the text within the button.

Add a mod property to make it easy to modify the margin-block-start of the icon, in case of alignment issues with icons that are not the intended workflow sizes.

lazd · 2024-07-31T21:26:09Z

FYI this one caught us by surprise over in Project X, we got unexpectedly wrapping buttons and misaligned icons!

We're disabling this wrapping behavior by default as it introduces bugs and putting it behind a [wrap] attribute for the fix. see PR https://git.corp.adobe.com/Horizon/hz/pull/89334

jawinn force-pushed the jawinn/css-621-button-wrapping-text branch from 3383b26 to e28f7b7 Compare October 31, 2023 20:00

github-actions bot temporarily deployed to pull request October 31, 2023 20:05 Inactive

jawinn force-pushed the jawinn/css-621-button-wrapping-text branch from e28f7b7 to eebd23f Compare October 31, 2023 20:14

github-actions bot temporarily deployed to pull request October 31, 2023 20:20 Inactive

jawinn added the run_vrt For use on PRs looking to kick off VRT label Oct 31, 2023

github-actions bot removed the run_vrt For use on PRs looking to kick off VRT label Oct 31, 2023

jenndiaz suggested changes Nov 1, 2023

View reviewed changes

components/button/stories/button.stories.js Outdated Show resolved Hide resolved

mdt2 reviewed Nov 1, 2023

View reviewed changes

components/button/index.css Show resolved Hide resolved

components/button/stories/button.stories.js Outdated Show resolved Hide resolved

github-actions bot temporarily deployed to pull request November 1, 2023 21:43 Inactive

jawinn requested a review from jenndiaz November 1, 2023 21:53

pfulton approved these changes Nov 2, 2023

View reviewed changes

jenndiaz approved these changes Nov 2, 2023

View reviewed changes

castastrophe force-pushed the jawinn/css-621-button-wrapping-text branch from f3ca374 to 9d325fa Compare November 3, 2023 18:34

github-actions bot temporarily deployed to pull request November 3, 2023 18:40 Inactive

jawinn force-pushed the jawinn/css-621-button-wrapping-text branch from 9d325fa to 4c4ab73 Compare November 8, 2023 21:27

github-actions bot temporarily deployed to pull request November 8, 2023 21:33 Inactive

jawinn added the run_vrt For use on PRs looking to kick off VRT label Nov 8, 2023

github-actions bot removed the run_vrt For use on PRs looking to kick off VRT label Nov 8, 2023

jawinn force-pushed the jawinn/css-621-button-wrapping-text branch from 4c4ab73 to 7f5300d Compare November 8, 2023 22:14

github-actions bot temporarily deployed to pull request November 8, 2023 22:17 Inactive

jawinn added the run_vrt For use on PRs looking to kick off VRT label Nov 8, 2023

github-actions bot removed the run_vrt For use on PRs looking to kick off VRT label Nov 8, 2023

jawinn added the do not merge A flag for a branch indicating it should not be merged. label Nov 9, 2023

castastrophe reviewed Nov 14, 2023

View reviewed changes

jawinn force-pushed the jawinn/css-621-button-wrapping-text branch from 7ace8d0 to 49ba01d Compare November 16, 2023 16:59

castastrophe marked this pull request as draft January 17, 2024 14:17

jawinn force-pushed the jawinn/css-621-button-wrapping-text branch from e111b4e to 1a3df2a Compare February 7, 2024 23:23

jawinn marked this pull request as ready for review February 8, 2024 19:16

jawinn added ready-for-review and removed do not merge A flag for a branch indicating it should not be merged. labels Feb 9, 2024

jawinn force-pushed the jawinn/css-621-button-wrapping-text branch from b6bbbd8 to bf3688d Compare February 14, 2024 22:45

castastrophe force-pushed the jawinn/css-621-button-wrapping-text branch from 28e2929 to 574a34f Compare February 16, 2024 16:24

castastrophe mentioned this pull request Feb 16, 2024

feat(storybook): add a shared decorator for group rendering and static color settings #2420

Closed

6 tasks

castastrophe approved these changes Feb 16, 2024

View reviewed changes

castastrophe force-pushed the jawinn/css-621-button-wrapping-text branch from 574a34f to e84b131 Compare February 16, 2024 16:29

castastrophe added ready-to-merge and removed ready-for-review labels Feb 16, 2024

castastrophe enabled auto-merge (squash) February 16, 2024 16:35

castastrophe force-pushed the jawinn/css-621-button-wrapping-text branch 3 times, most recently from 5a820f8 to a517b27 Compare February 16, 2024 17:33

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

jawinn added 6 commits February 20, 2024 13:20

fix(button): restore ability for button text to wrap

869c246

Remove the CSS that prevents the text in the button from wrapping. The guidelines for the component say "When the button text is too long for the horizontal space available, it wraps to form another line."

docs(button): add wrapping text story

7d73a9c

Add "Wrapping" story that demonstrates a button with long text that wraps to the next line. Some template adjustments and additional args added to allow for this.

docs(button): rename storybook control to layout

c80678b

Rename Storybook control for stacking buttons to "layout" with specified options, which is a more appropriate name. The control has also been made visible.

feat(button): mod property for icon margin top

c81a6f2

Add a mod property to make it easy to modify the margin-block-start of the icon, in case of alignment issues with icons that are not the intended workflow sizes.

pfulton force-pushed the jawinn/css-621-button-wrapping-text branch from a517b27 to c81a6f2 Compare February 20, 2024 18:20

pfulton added the run_vrt For use on PRs looking to kick off VRT label Feb 20, 2024

castastrophe merged commit 3f14a86 into main Feb 20, 2024

castastrophe deleted the jawinn/css-621-button-wrapping-text branch February 20, 2024 18:28

github-actions bot mentioned this pull request Apr 26, 2024

chore: release (next) #2706

Merged

rubencarvalho mentioned this pull request Oct 2, 2024

feat(button): add noWrap property adobe/spectrum-web-components#4779

Merged

11 tasks

fix(button): support for wrapping text #2248

fix(button): support for wrapping text #2248

Uh oh!

Conversation

jawinn commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

Uh oh!

github-actions bot commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jenndiaz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdt2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pfulton left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File metrics

Summary

button

Uh oh!

castastrophe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

castastrophe Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

jawinn Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

castastrophe Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jawinn commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jawinn commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfulton commented Feb 14, 2024

Uh oh!

castastrophe Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

castastrophe Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

lazd commented Jul 31, 2024

Uh oh!

Uh oh!

jawinn commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Nov 10, 2023 •

edited

Loading

jawinn commented Nov 21, 2023 •

edited

Loading

jawinn commented Feb 8, 2024 •

edited

Loading