Skip to content

fix(fieldlabel): prevent required icon from appearing on its own line when wrapping#3652

Merged
cdransf merged 1 commit into
spectrum-twofrom
cdransf/fieldlabel-required-wrap-fix
Apr 16, 2025
Merged

fix(fieldlabel): prevent required icon from appearing on its own line when wrapping#3652
cdransf merged 1 commit into
spectrum-twofrom
cdransf/fieldlabel-required-wrap-fix

Conversation

@cdransf
Copy link
Copy Markdown
Member

@cdransf cdransf commented Apr 1, 2025

Description

Addresses case where fieldlabel required icon could appear on its own line when wrapping by applying text-wrap: pretty; to the parent label class.

Validation steps

  1. Open the storybook link for the PR.
  2. Navigate to the fieldlabel component.
  3. Enable the Required control.
  4. Adjust the contents of the Label control and verify that the Required icon does not render on a line separate from the text when it wraps.

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

@cdransf cdransf added size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. skip_vrt Add to a PR to skip running VRT (but still pass the action) ready-for-review labels Apr 1, 2025
@cdransf cdransf self-assigned this Apr 1, 2025
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 1, 2025

🦋 Changeset detected

Latest commit: 2c0e7ac

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/fieldlabel Patch

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

@cdransf cdransf force-pushed the cdransf/fieldlabel-required-wrap-fix branch from 83f108e to cbe6e81 Compare April 1, 2025 14:36
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 1, 2025

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 1, 2025

File metrics

Summary

Total size: 1.37 MB*

Package Size Minified Gzipped
fieldlabel 5.22 KB 4.98 KB 1.14 KB

fieldlabel

Filename Head Minified Gzipped Compared to base
index.css 5.22 KB 4.98 KB 1.14 KB 🔴 ⬆ 0.02 KB
metadata.json 2.89 KB - - -
* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@cdransf cdransf force-pushed the cdransf/fieldlabel-required-wrap-fix branch 3 times, most recently from 7705dba to a0903af Compare April 2, 2025 15:45
Copy link
Copy Markdown
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

I was so ready to throw an approval on this, but I opened the textfield testing grid to look at the side label, and I'm still getting an asterisk on a separate line:

image

Wondering if we need a different approach?

Comment thread .changeset/honest-animals-stop.md Outdated
@cdransf cdransf force-pushed the cdransf/fieldlabel-required-wrap-fix branch from a0903af to 24aa687 Compare April 4, 2025 22:45
@rise-erpelding rise-erpelding self-requested a review April 7, 2025 14:50
Copy link
Copy Markdown
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

Looks good! This works in the field label story and also appears to work fine in textfield, slider, progressbar, picker, combobox, fieldgroup, and form as far as I can tell! The only recommendation I have would be to move the text-wrap: pretty so it sits with other similar style declarations instead of with the custom prop declarations.

Comment thread components/fieldlabel/index.css Outdated
Comment thread .changeset/honest-animals-stop.md Outdated
@jawinn jawinn self-requested a review April 7, 2025 15:13
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.

There are some issues with changing this to flex as noted here.
Are we sure about that sort of layout change here and preventing wrapping? This could have other negative consequences, with side label fields forced to be wider than before and if there were other elements within the label.

I'm not sure if there is a way to adjust it to fix the alignment issues mentioned and I'm wondering if the non-breaking space should still be recommended for the implementation? Since AFAIK the asterisk still needs to wrap along with the text to be inline at the end of the text.

@cdransf cdransf force-pushed the cdransf/fieldlabel-required-wrap-fix branch 2 times, most recently from 3e1d4ac to aa46f10 Compare April 7, 2025 17:17
@cdransf cdransf requested review from jawinn and rise-erpelding April 7, 2025 17:18
Copy link
Copy Markdown
Collaborator

@rise-erpelding rise-erpelding 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   solution with text-wrap: pretty; will work well! I think we just need to do a little cleanup on the code. 🙂

Comment thread components/fieldlabel/index.css
Comment thread components/fieldlabel/index.css Outdated
Comment on lines +28 to +30
}

.spectrum-FieldLabel {
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.

Do we need to add this here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not necessarily! I was looking to split out custom properties from rules which resulted in three separate blocks. I pulled the styles from line ~88 up into this block. Happy to rearrange as needed.

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.

It looks like things are currently a little out of order with how I would expect to match up with how it is with the other components. You're right that custom properties should be separate and at the top.

I think Rise was suggesting that previously a single .spectrum-FieldLabel block at the top could have the CJK and disabled lines within it. So there'd be two .spectrum-FieldLabel blocks instead of three. Then all the style declarations would go back where they were in a .spectrum-FieldLabel block, after the size and static color classes. Gosh, trying to explain where to move around code blocks via text is cumbersome 😆

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

😅 I think things are in the right order now.

Comment thread components/fieldlabel/index.css Outdated
Comment thread components/fieldlabel/stories/fieldlabel.stories.js Outdated
@cdransf cdransf force-pushed the cdransf/fieldlabel-required-wrap-fix branch from 412082e to c72a363 Compare April 14, 2025 15:30
Comment thread components/fieldlabel/index.css Outdated
@cdransf cdransf force-pushed the cdransf/fieldlabel-required-wrap-fix branch 2 times, most recently from ba891dd to 91e69d1 Compare April 14, 2025 21:53
@cdransf cdransf requested a review from rise-erpelding April 14, 2025 21:55
Copy link
Copy Markdown
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

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

Sorry, one tiny thing with that display: block removal!

Comment thread components/fieldlabel/index.css
@cdransf cdransf force-pushed the cdransf/fieldlabel-required-wrap-fix branch from 91e69d1 to a2fa5a4 Compare April 15, 2025 23:51
@cdransf cdransf requested a review from rise-erpelding April 15, 2025 23:51
@cdransf cdransf force-pushed the cdransf/fieldlabel-required-wrap-fix branch from a2fa5a4 to 2c0e7ac Compare April 15, 2025 23:52
@cdransf cdransf merged commit 8b4511e into spectrum-two Apr 16, 2025
12 checks passed
@cdransf cdransf deleted the cdransf/fieldlabel-required-wrap-fix branch April 16, 2025 16:24
@github-actions github-actions Bot mentioned this pull request Apr 16, 2025
@castastrophe castastrophe mentioned this pull request Aug 6, 2025
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. skip_vrt Add to a PR to skip running VRT (but still pass the action)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants