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

Jump to bottom

chore(Tile): updated props for v11 #9628

Merged
merged 10 commits into from Sep 20, 2021
Merged

chore(Tile): updated props for v11 #9628

merged 10 commits into from Sep 20, 2021

Conversation

sstrubberg
Copy link
Member

@sstrubberg sstrubberg commented Sep 9, 2021 •
edited

REF #9535

Changelog

  • Added deprecation warnings for handleClick and handleKeyDown to Tile.
  • Updated the PublicAPI test snapshot.
  • Touched up Tile by changing {...other} -> {...rest} and classNames -> cx.
  • Updated the Tile story with new props.
  • Added comments in the appropriate areas to remove deprecations for the next major release.

Testing / Reviewing

  • Check the Tile stories and make sure everything is firing off as it should.
  • Make sure tests are passing.

@netlify
Copy link

netlify bot commented Sep 9, 2021 •
edited

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 11a5dc5

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6148b728e767cc0007d4474e

😎 Browse the preview: https://deploy-preview-9628--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Sep 9, 2021 •
edited

✔️ Deploy Preview for carbon-components-react ready!

🔨 Explore the source changes: 11a5dc5

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/6148b7284548e10007d2eb4f

😎 Browse the preview: https://deploy-preview-9628--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Sep 9, 2021 •
edited

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 11a5dc5

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/6148b728593b4900085bc670

😎 Browse the preview: https://deploy-preview-9628--carbon-elements.netlify.app

@sstrubberg sstrubberg marked this pull request as ready for review September 9, 2021 20:02
@sstrubberg sstrubberg requested review from a team as code owners September 9, 2021 20:02
@sstrubberg sstrubberg requested review from joshblack, jnm2377, vpicone and aledavila and removed request for jnm2377 September 9, 2021 20:02
@sstrubberg sstrubberg changed the title chore(prop-changes): updated Button and Tile props for v11 chore(Tile): updated props for v11 Sep 13, 2021
vpicone
vpicone reviewed Sep 15, 2021
View reviewed changes
packages/react/src/components/Tile/Tile.js Outdated
@@ -549,7 +579,8 @@ export class ExpandableTile extends Component {
},
() => {
this.setMaxHeight();
this.props.handleClick(evt);
// TODO: Remove handleClick prop when handleClick is deprecated
this.props.handleClick?.(evt) || this.props.onClick(evt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the same optional chaining exist for onClick so we only call it if they pass it in? Would prevent the need for the no-op defaultProp functions.

vpicone
vpicone reviewed Sep 15, 2021
View reviewed changes
packages/react/src/components/Tile/Tile.js Outdated
@@ -574,20 +605,20 @@ export class ExpandableTile extends Component {
tileMaxHeight, // eslint-disable-line
tilePadding, // eslint-disable-line
handleClick, // eslint-disable-line
onClick,
onKeyUp,
onClick, // eslint-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth adding a note why its disabled so we can change the rule or fix it in the future

vpicone
vpicone reviewed Sep 15, 2021
View reviewed changes
Copy link
Contributor

@vpicone vpicone left a comment

Choose a reason for hiding this comment

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

Couple quick questions

joshblack
joshblack suggested changes Sep 15, 2021
View reviewed changes
packages/react/src/components/Tile/Tile.js Outdated Show resolved Hide resolved
packages/react/src/components/Tile/Tile.js Outdated Show resolved Hide resolved
vpicone
vpicone approved these changes Sep 20, 2021
View reviewed changes
packages/react/src/components/Tile/Tile.js Outdated
@@ -196,10 +196,8 @@ export class ClickableTile extends Component {
href={href}
className={classes}
{...rest}
// TODO: replace with onClick when handleClick prop is deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually didn't mind these comments, @joshblack do you have a preference for tracking things we'll need to rip out post-v11.

@sstrubberg sstrubberg merged commit a803128 into carbon-design-system:main Sep 20, 2021
@sstrubberg sstrubberg deleted the prop-changes-alt branch September 20, 2021 19:54
@joshblack joshblack mentioned this pull request Sep 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshblack joshblack joshblack approved these changes

@vpicone vpicone vpicone approved these changes

@aledavila aledavila Awaiting requested review from aledavila

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sstrubberg @joshblack @vpicone