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

[docs] Add Tooltip TypeScript demos #15061

Merged
merged 2 commits into from
Mar 30, 2019
Merged

[docs] Add Tooltip TypeScript demos #15061

merged 2 commits into from
Mar 30, 2019

Conversation

Dudrie
Copy link
Contributor

@Dudrie Dudrie commented Mar 26, 2019

Here are the TypeScript demos for the tooltip components. A few of them did not need additional typing, so I did just create the corresponding tsx file for them.

However, I had a question while doing so: In CustomizedTooltip useState is used to create a ref. Is there a reason why the useRef hook is not used there?

@Dudrie Dudrie changed the title Add Tooltips TypeScript demos Add Tooltip TypeScript demos Mar 26, 2019
@Dudrie Dudrie changed the title Add Tooltip TypeScript demos [docs] Add Tooltip TypeScript demos Mar 26, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Mar 26, 2019

No bundle size changes comparing 6981a04...375336f

Generated by 🚫 dangerJS against 375336f


function CustomizedTooltips() {
const classes = useStyles();
const [arrowRef, setArrowRef] = React.useState<HTMLSpanElement | null>(null);
Copy link
Member

Choose a reason for hiding this comment

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

@Dudrie Try implementing the same feature with

Suggested change
const [arrowRef, setArrowRef] = React.useState<HTMLSpanElement | null>(null);
const arrowRef = React.useRef<HTMLSpanElement>(null);

You will probably need quite a bit of additional code to handle re-rendering.

The current implementation is a smart way to avoid creating ref callbacks and forceUpdate methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, useRef does not trigger a re-render if it's current changes. Using this hook would probably add more boilerplate code.

Copy link
Member

Choose a reason for hiding this comment

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

Honestly it's one of the reasons I don't like the ref suffix. When used right it doesn't add information but if used wrong it adds confusion. Simply using arrowNode would've been clearer and then it would also be obvious that setArrowNode should only be used in ref props for host or ref forwarding components.

But there's no reason to change this now. Discussions regarding this haven't been very helpful to either side anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I forgot to comment on the ref suffix situation last time. For me I don’t think the ref suffix is needed most of the time. I guess I would use it when passing something to the ref prop.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation typescript labels Mar 26, 2019
@oliviertassinari
Copy link
Member

@Dudrie Thank you!

@Dudrie Dudrie deleted the ts-demos-tooltips branch March 30, 2019 10:33
@eps1lon eps1lon mentioned this pull request Mar 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants