Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(segmented-control-item): allow displaying and icon when items are empty with a start/end icon #9300

Conversation

jcfranco
Copy link
Member

@jcfranco jcfranco commented May 9, 2024

Related Issue:* #6413

Summary

This updates segmented-control-item to display a centered icon when specified and the item is empty.

Note: this removes using value as a fallback label as non-breaking for the following reasons:

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label May 9, 2024
@jcfranco jcfranco marked this pull request as ready for review May 9, 2024 22:07
@jcfranco jcfranco requested a review from a team as a code owner May 9, 2024 22:07
@jcfranco jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label May 9, 2024
Copy link
Contributor

@alisonailea alisonailea left a comment

Choose a reason for hiding this comment

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

Love the simplification!

/**
* Indicates whether the text is displayed.
*/
@Prop({ reflect: true }) textDisabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we put this on the individual items? Not sure the parent needs to have knowledge of this. There are occasionally times when you'd want to display the active Segmented Control Item's text and not the others.

I know we probably avoided it for breaking change reasons - but I wonder if in the future we can just use the presence of slotted text to determine this instead of having a property - we do this on button, chip, etc., to conditionally display padding when in "icon only state". It might also be nice to have a separate (and required) "label" property for value for AT reasons?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can forego the prop in favor of slotted content based on some findings I shared in the issue:

OK, so I tracked down the behavior I called out in my earlier comment and it looks like using value as the fallback label is intentional, but there is no explicit spec for it in the original issue, PR nor documentation. It's sort of an outlier and also breaks if there's any whitespace. Also, value might not be the best option for a user-friendly label (e.g., casing, localization). For these reasons, I think it's safe to drop to simplify the behavior w/o it constituting a breaking change.

@jcfranco jcfranco changed the title feat(segmented-control): add icon-only mode via text-disabled prop feat(segmented-control): allow displaying and icon when items are empty with a specified icon May 10, 2024
@jcfranco jcfranco changed the title feat(segmented-control): allow displaying and icon when items are empty with a specified icon feat(segmented-control-item): allow displaying and icon when items are empty with a start/end icon May 10, 2024
@jcfranco jcfranco added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels May 13, 2024
@jcfranco jcfranco merged commit 9fc610d into main May 13, 2024
13 checks passed
@jcfranco jcfranco deleted the jcfranco/6413-update-segmented-control-to-support-icon-only-configuration branch May 13, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants