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): added new prop titleText to specify title on clear #4163

Merged

Conversation

ankitious
Copy link
Contributor

@ankitious ankitious commented Oct 2, 2019

Closes #4025

Changelog

New

  • Added titleText prop to specify title text on clear button

Testing / Reviewing

  • Go to Tab with filter
  • Specify titleText prop as a string
  • hover over clear button, text should be visible.

@ankitious ankitious requested a review from a team as a code owner October 2, 2019 07:25
@ghost ghost requested review from emyarod and joshblack October 2, 2019 07:25
@netlify
Copy link

netlify bot commented Oct 2, 2019

Deploy preview for the-carbon-components ready!

Built with commit f0af3c0

https://deploy-preview-4163--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Oct 2, 2019

Deploy preview for carbon-components-react ready!

Built with commit f0af3c0

https://deploy-preview-4163--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Oct 2, 2019

Deploy preview for carbon-elements ready!

Built with commit f0af3c0

https://deploy-preview-4163--carbon-elements.netlify.com

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.

for the sake of consistency can we refactor this to follow the translateWithId pattern we have in the rest of the library?

for reference: https://github.com/carbon-design-system/carbon/blob/master/packages/react/src/components/ListBox/ListBoxField.js

@asudoh
Copy link
Contributor

asudoh commented Oct 2, 2019

@emyarod translateWithId is there only for limited set of components - So probably we can leave it as-is?

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Ignore me!

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Would we want to use the title prop directly for this? Wasn't sure if it's being used elsewhere in the component but would be fine to use it directly

@ankitious
Copy link
Contributor Author

@joshblack title is not used anywhere else in component, so I have replaced titleText to title prop. Thanks!

@ankitious
Copy link
Contributor Author

@joshblack I have changed props name to title, or do we want to follow translateWithId pattern suggested by @emyarod

Please provide your suggestion. Thanks!

@joshblack
Copy link
Contributor

@ankitious should be good with title here, as I think it's analogous to how we expose labels for folks to customize 😄

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, just one small nitpick

packages/react/src/components/Tag/Tag.js Outdated Show resolved Hide resolved
@joshblack joshblack requested a review from dakahn October 16, 2019 20:11
@joshblack
Copy link
Contributor

cc @dakahn including you to double-check a11y on this if you have a sec, I think the component itself may already have an issue given that there is no button for the icon. If there are issues for this, and we're planning on addressing them in the a11y project, does the proposed solution here align with whatever the ideal is? Trying to think if we should be using aria-label here now, or not.

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

This is fine. Being able to customize the title attribute doesn't really jam up anything we had planned. The aria seems to be adequate in so far as it's read properly by NVDA on Firefox.

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 doesn't offer internationalization / localization hook
6 participants