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

fix(tag): add title to tags #6112

Merged
merged 8 commits into from
May 28, 2020

Conversation

tw15egan
Copy link
Collaborator

@tw15egan tw15egan commented May 19, 2020

Closes #6111

The title attribute on Tag allows the user to customize to text that appears on the aria-label for the close icon. This PR adds a boolean prop, enableTagTitle, that when enabled takes the value of the tag and will display it on hover.

Changelog

New

  • enableTagTitle prop to allow for a title attribute to be added on the Tag

Changed

  • title now is added to the close icon, if provided. Previously was only being added as an aria-label, which due to the naming seems confusing.
  • Text updates to the Tag story

Testing / Reviewing

Enable/disable the new prop, ensure it works properly

@tw15egan tw15egan requested a review from a team as a code owner May 19, 2020 19:09
@ghost ghost requested review from asudoh and dakahn May 19, 2020 19:10
@tw15egan tw15egan requested review from a team and jeanservaas and removed request for a team May 19, 2020 19:10
@netlify
Copy link

netlify bot commented May 19, 2020

Deploy preview for carbon-elements ready!

Built with commit daf9563

https://deploy-preview-6112--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented May 19, 2020

Deploy preview for carbon-components-react ready!

Built with commit daf9563

https://deploy-preview-6112--carbon-components-react.netlify.app

@tw15egan tw15egan requested a review from a team as a code owner May 19, 2020 20:06
@joshblack
Copy link
Contributor

@tw15egan would we want to set the title by default, or nah?

@tw15egan
Copy link
Collaborator Author

tw15egan commented May 21, 2020

@joshblack doesn't make much of a difference to me, figured it would be best to opt into this feature than to make it default

@emyarod
Copy link
Member

emyarod commented May 21, 2020

I'm not sure if there are additional a11y reasons for only setting the title to filter tags (#4163, #4863) but if we do want to apply it to non filter tags (and it doesn't have any a11y impact), I think it would be better to keep the API surface area low and set it by default. it seems there is a workaround already according to the original issue author so just want to double check why title is specific to filter tags at the moment. my guess is because we don't encourage truncating tags by default?

@asudoh asudoh requested a review from emyarod May 21, 2020 23:40
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM basically, assuming that we have a condition where doesn't want the title hover. Thanks @tw15egan!

packages/react/src/components/Tag/Tag.js Outdated Show resolved Hide resolved
packages/react/src/components/Tag/Tag.js Outdated Show resolved Hide resolved
@tw15egan
Copy link
Collaborator Author

tw15egan commented May 22, 2020

@emyarod updated it to be enabled by default 👍

@asudoh added your changes 👍

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

does there need to be a prop for enabling the title? it should just automatically take the prop value right?

@tw15egan
Copy link
Collaborator Author

@emyarod do we want it always to be enabled with no way to remove it? I was just following convention like what we do with MultiSelect and useTitleInItem

@emyarod
Copy link
Member

emyarod commented May 22, 2020

for multiselect I believe that prop is originally from a few years ago and more recently we have been setting titles unconditionally (https://github.com/carbon-design-system/carbon/pull/5621/files #5102). but I think the context for those components is slightly different from tags

what I meant in my earlier comment (#6112 (comment)) was I think it makes more sense for the title logic to be identical (or similar) regardless of whether or not it is a filter tag. but if it makes sense to conditionally set the title only for non-filter tags then the new prop is fine

@tw15egan
Copy link
Collaborator Author

I'm good with that, I'll just set it by default and remove the prop

@tw15egan tw15egan changed the title fix(tag): add prop to enable title on tags fix(tag): add title to tags May 22, 2020
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍, wanted to have @emyarod's final review. Thanks @tw15egan for the update!

Copy link
Member

@emyarod emyarod 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 to me

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

🎉

@tw15egan tw15egan merged commit 7935ad4 into carbon-design-system:master May 28, 2020
@tw15egan tw15egan deleted the tag-title-update branch April 28, 2021 18:15
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.

Tag component only sets title if filter
6 participants