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

[material-ui][SpeedDial] Fix SpeedDial tooltip placement #41997

Open
wants to merge 5 commits into
base: next
Choose a base branch
from

Conversation

jjisabi
Copy link

@jjisabi jjisabi commented Apr 22, 2024

Fixes #41067

Closes #41414 (old inactive PR)

Before:
스크린샷 2024-04-11 034214

After:
스크린샷 2024-04-11 034329

I used the tooltip component instead of SpeedDialActionStaticTooltip. Then fixed the tooltip placement by changing the fab component style (the transform property changes tooltip placement).

Deploy preview: https://deploy-preview-41997--material-ui.netlify.app/material-ui/react-speed-dial/#persistent-action-tooltips.

@mui-bot
Copy link

mui-bot commented Apr 22, 2024

Netlify deploy preview

https://deploy-preview-41997--material-ui.netlify.app/

packages/material-ui/material-ui.production.min.js: parsed: -0.05% 😍, gzip: -0.05% 😍

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against a5a484d

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

The issue is already being addressed in #41414. Additionally, the modification you made constitutes a breaking change. The tooltip should remain white, as it was before.

@zannager zannager added component: speed dial This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Apr 22, 2024
@jjisabi
Copy link
Author

jjisabi commented Apr 22, 2024

@ZeeshanTamboli Hello! Thanks for the comment.
Is there any updates in #41414? or the issue has been solved with #41414? I can see it's still open.

Additionally, the modification you made constitutes a breaking change. The tooltip should remain white, as it was before.

I use tooltipPlacement prop in tooltip component.
So this is the default color of the tooltip component, but i think i can make it white.

@ZeeshanTamboli
Copy link
Member

Is there any updates in #41414? or the issue has been solved with #41414? I can see it's still open.

No new updates there. Feel free to address it in this PR.

@jjisabi
Copy link
Author

jjisabi commented Apr 23, 2024

@ZeeshanTamboli Thanks!
스크린샷 2024-04-23 143700
I make the tooltip white.
Btw, in speedDial, all other tooltips are black. Is there a reason why only this should be white?

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Apr 23, 2024

@ZeeshanTamboli Thanks! 스크린샷 2024-04-23 143700 I make the tooltip white.

Are you sure? I can't see it as white in the docs preview: Link to docs preview when hovering over it. It should match the white tooltip in production: Link to production.

Btw, in speedDial, all other tooltips are black. Is there a reason why only this should be white?

Not entirely sure, but for consistent user experience, persistent tooltips should remain white to avoid any unexpected breaking changes for existing apps using them with the tooltipOpen prop.

@jjisabi
Copy link
Author

jjisabi commented Apr 23, 2024

@ZeeshanTamboli It works now. Could you please check again?

Not entirely sure, but for consistent user experience, persistent tooltips should remain white to avoid any unexpected breaking changes for existing apps using them with the tooltipOpen prop.

I got it. Thank you for letting me know.

@ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli It works now. Could you please check again?

Now it's white, but the size is smaller. You can see in the documentation preview I provided in the PR description. It should match the previous appearance exactly. Why not reuse the previous styles? This PR should only address the tooltip's positioning. Additionally, it should maintain the previous animation when opened.

@ZeeshanTamboli ZeeshanTamboli added the bug 🐛 Something doesn't work label Apr 23, 2024
@jjisabi
Copy link
Author

jjisabi commented Apr 26, 2024

@ZeeshanTamboli
All styles, except for the animation, match the previous version. Could you please check?

When scale animation(previous animation) maintain, the tooltip finds the anchored element and is placed before the anchored element is placed. So the tooltip can't be placed in the correct position. Because of this issue, I remove the animation.

I tried to find a way to maintain it, but couldn't find it. Do you have any other suggestions or ways for this?

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@jjisabi There are a lot of file changes related to pigment-css. Could you remove them?

@jjisabi jjisabi force-pushed the fix-speedDial-tooltip-#41067 branch from 5241dca to 5609871 Compare May 1, 2024 12:30
@jjisabi
Copy link
Author

jjisabi commented May 1, 2024

@ZeeshanTamboli sorry, my bad. I removed them all.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@jjisabi Thanks. Can you address the failing CI? Also, Argos CI flagged some unexpected changes, which needs attention.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 6, 2024
with '#' will be ignored, and an empty message aborts the cmmit.
rebase SpeedDialAction.js
@jjisabi jjisabi force-pushed the fix-speedDial-tooltip-#41067 branch from 0880ac7 to a5a484d Compare May 8, 2024 07:58
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 8, 2024
onClose={handleTooltipClose}
onOpen={handleTooltipOpen}
open={open && tooltipOpenProp}
classes={TooltipClasses}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
classes={TooltipClasses}
classes={tooltipClasses}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: speed dial This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][SpeedDial] Bug with right/left direction in persistent action tooltips
4 participants