Skip to content

feat(button): add label text wrap option#3086

Merged
aramos-adobe merged 23 commits into
mainfrom
aramos-adobe/button-wrap-prop
Sep 23, 2024
Merged

feat(button): add label text wrap option#3086
aramos-adobe merged 23 commits into
mainfrom
aramos-adobe/button-wrap-prop

Conversation

@aramos-adobe
Copy link
Copy Markdown
Contributor

Description

  • This change makes buttons not wrap label text by default.
  • Introducing a class to make label text wrap if dev chooses to do so.
  • Added label wrap class in Text Overflow story in 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

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. 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

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. ✨

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Sep 9, 2024

🦋 Changeset detected

Latest commit: 1308f38

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@spectrum-css/button Minor

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 9, 2024

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 9, 2024

File metrics

Summary

Total size: 4.11 MB*
Total change (Δ): ⬆ 0.61 KB (0.01%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
button 84.16 KB ⬆ 0.20 KB

Details

button

File Head Base Δ
index-base.css 54.20 KB 54.01 KB ⬆ 0.20 KB (0.37%)
index-theme.css 30.56 KB 30.56 KB -
index-vars.css 84.16 KB 83.96 KB ⬆ 0.20 KB (0.23%)
index.css 84.16 KB 83.96 KB ⬆ 0.20 KB (0.23%)
themes/express.css 29.37 KB 29.37 KB -
themes/spectrum.css 29.43 KB 29.43 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.

@aramos-adobe
Copy link
Copy Markdown
Contributor Author

Screenshot 2024-09-11 at 9 27 06 AM Screenshot 2024-09-11 at 9 27 15 AM Screenshot 2024-09-11 at 9 27 29 AM

Copy link
Copy Markdown
Collaborator

@jawinn jawinn left a comment

Choose a reason for hiding this comment

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

I think the nowrap CSS should work here. This and the added control look good!

Could you make sure that we have this included in the VRTs, by adding a noWrap: true test in the button.test.js file? It will need either a set width on the wrapperStyles, or a min-inline-size like the text overflow examples in order for wrapping to happen. When added it'll be viewable on the Default story with "Show testing preview" turned on.

Comment thread .changeset/mean-shrimps-press.md Outdated
Comment thread components/button/stories/template.js Outdated
Comment thread components/button/stories/template.js Outdated
Comment thread components/button/stories/button.stories.js Outdated
Comment thread components/button/stories/button.stories.js Outdated
@aramos-adobe aramos-adobe requested a review from jawinn September 13, 2024 20:46
Comment thread components/button/index.css Outdated

/* Disable button label wrap */
.spectrum-Button--noWrap {
justify-content: flex-start;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With the overflow and text-overflow additions, is this addition still needed? With short text plus the minimum width and for buttons with some other set width, the text will no longer be centered:
Screenshot 2024-09-16 at 3 10 40 PM

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jawinn When the max-inline-size is set all the button content gets cut off from left and right edges of the container. I was thinking of doing a flex-start and overflow additions it'll show the dev the text is too long and they need to make an adjustment character wise.

After some thought, I'm going to remove this because it's not a design specification and I don't want teams to get mislead in anyway.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I do think what you had, .spectrum-Button-label { overflow: hidden; text-overflow: ellipsis; } is a good idea. I think we should handle the text overflow for cases where the button's width is constrained, as it gets a bit broken as seen in the Chromatic testing view right now. Does it work okay with just those two properties?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jawinn Yes, they work with just these two properties.

Comment thread .changeset/green-maps-end.md Outdated
Comment thread components/button/index.css
Comment thread components/button/stories/button.stories.js
Comment thread components/button/stories/button.test.js
@aramos-adobe aramos-adobe requested a review from jawinn September 17, 2024 15:09
@pfulton pfulton added the s1 label Sep 17, 2024
staticColor,
noWrap: {
name: "Disable label wrap",
description: "Used only to keep the button label text in one line. It is intended to have the label wrap as it is part of design specifications.",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: "Used only to keep the button label text in one line. It is intended to have the label wrap as it is part of design specifications.",
description: "Used to keep the button label text on one line. Note that this option is not a part of the design specifications which intend for the label to wrap. Use with care and consideration of this option's overflow behavior and the readability of the button's content.",

Comment thread .changeset/green-maps-end.md Outdated
Comment on lines +5 to +7
Adds the ability to disable text wrapping within the button component's label by applying the class `.spectrum-Button--noWrap` to `.spectrum-Button`.

If the text is longer than the button container an ellipsis `...` indicator will show. Please adjust text length and `max-inline-size` of the button container.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Adds the ability to disable text wrapping within the button component's label by applying the class `.spectrum-Button--noWrap` to `.spectrum-Button`.
If the text is longer than the button container an ellipsis `...` indicator will show. Please adjust text length and `max-inline-size` of the button container.
Adds the ability to disable text wrapping within the button component's label by applying the class `.spectrum-Button--noWrap` to `.spectrum-Button`. If the text overflows the maximum allowed width of the button, an ellipsis `...` indicator will show. Please note that this option is not a part of the design specification and should be used carefully, with consideration of this overflow behavior and the readability of the button's content.

I was unclear about what the last "Please..." sentence was suggesting the user should do. I took a shot at rewording a bit, with a bit more warning.

Comment thread components/button/stories/button.stories.js Outdated
Comment thread components/button/stories/button.stories.js
Copy link
Copy Markdown
Collaborator

@pfulton pfulton left a comment

Choose a reason for hiding this comment

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

Thanks!!

Comment thread .changeset/mean-shrimps-press.md Outdated
@@ -0,0 +1,5 @@
---
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh interesting that two of these got generated. I think we could delete one of them, as they're signifying the same changes. Maybe remove this one.

@aramos-adobe
Copy link
Copy Markdown
Contributor Author

image

@aramos-adobe aramos-adobe merged commit 088c104 into main Sep 23, 2024
@aramos-adobe aramos-adobe deleted the aramos-adobe/button-wrap-prop branch September 23, 2024 12:39
@github-actions github-actions Bot mentioned this pull request Sep 23, 2024
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.

3 participants