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( radio-tile, selectable-tile): deprecates iconDescription and adds handleOnChange function #4966

Merged
merged 5 commits into from
Jan 8, 2020

Conversation

abbeyhrt
Copy link
Contributor

@abbeyhrt abbeyhrt commented Jan 7, 2020

Closes #3474

This was done during mob programming with @aledavila and @joshblack.

This PR adds a handleOnChange to the SelectableTile so that the tiles are selectable with Jaws and VO. It changes the name of the "Selectable Tile" story to "Radio" since the example uses RadioTile and causes confusion with the MultiSelect story that does use the SelectableTile. It also deprecates iconDescription for SelectableTile and RadioTile and removes its implementation. The CheckmarkFilled icon is purely for decoration and removing the iconDescription allows it to be ignored by screenreaders. This PR also converts RadioTile to a functional component instead of a class.

Changelog

New

  • handleOnChange function for SelectableTile

Changed

  • RadioTile to function
  • Selectable Tile story changed to Radio Tile story

Removed

  • iconDescription deprecated for RadioTile and SelectableTile

Testing / Reviewing

Verify that with VO and Jaws that you can check and uncheck all of the selectable tiles and generally check to make sure everything is still working as expected.

@abbeyhrt abbeyhrt requested a review from a team as a code owner January 7, 2020 22:33
@ghost ghost requested review from aledavila and joshblack January 7, 2020 22:33
@netlify
Copy link

netlify bot commented Jan 7, 2020

Deploy preview for the-carbon-components ready!

Built with commit c91f70b

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

@netlify
Copy link

netlify bot commented Jan 7, 2020

Deploy preview for carbon-components-react failed.

Built with commit 0ba84bc

https://app.netlify.com/sites/carbon-components-react/deploys/5e15073c115fd000071a0998

@netlify
Copy link

netlify bot commented Jan 7, 2020

Deploy preview for carbon-elements failed.

Built with commit 0ba84bc

https://app.netlify.com/sites/carbon-elements/deploys/5e15073cd24e0d0008e8128f

@netlify
Copy link

netlify bot commented Jan 7, 2020

Deploy preview for carbon-components-react failed.

Built with commit c91f70b

https://app.netlify.com/sites/carbon-components-react/deploys/5e163cad6f210d0009ce68f8

@netlify
Copy link

netlify bot commented Jan 7, 2020

Deploy preview for carbon-elements failed.

Built with commit c91f70b

https://app.netlify.com/sites/carbon-elements/deploys/5e163cadb3e5f1000a459ef1

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, just one thing - Thanks @abbeyhrt!

@@ -297,10 +306,12 @@ export class SelectableTile extends Component {
value,
name,
title,
// eslint-disable-next-line no-unused-vars
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of ignoring no-unused-vars here, we can safely remove iconDescription here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that but it seemed to cause this warning to pop up if iconDescription is defined:
Screen Shot 2020-01-08 at 10 34 17 AM
Seems like if folks are still using iconDescription the prop will be spread onto ...other and cause the warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

Though this has been merged already - You are right. I don't see it's a problem, though, given it's a React warning that doesn't prevent user applications from working, and they can simply remove iconDescription to get rid of the warning.

Copy link
Contributor

@aledavila aledavila left a comment

Choose a reason for hiding this comment

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

🎉

@joshblack joshblack merged commit 81af245 into carbon-design-system:master Jan 8, 2020
joshblack added a commit to joshblack/carbon that referenced this pull request Jan 13, 2020
…andleOnChange function (carbon-design-system#4966)

* fix(radio-tile,tile): refactor radio tile to fn and add onChange to tile

* chore(tile): removes default prop for deprecated icon description

Co-authored-by: Josh Black <[email protected]>
joshblack added a commit to joshblack/carbon that referenced this pull request Jan 14, 2020
…andleOnChange function (carbon-design-system#4966)

* fix(radio-tile,tile): refactor radio tile to fn and add onChange to tile

* chore(tile): removes default prop for deprecated icon description

Co-authored-by: Josh Black <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants