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

[Popper] Render function placement argument is not updated #17309

Closed
capsule5 opened this issue Sep 4, 2019 · 5 comments · Fixed by #17781
Closed

[Popper] Render function placement argument is not updated #17309

capsule5 opened this issue Sep 4, 2019 · 5 comments · Fixed by #17781
Labels
bug 🐛 Something doesn't work component: Popper The React component. See <Popup> for the latest version. good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/

Comments

@capsule5
Copy link

capsule5 commented Sep 4, 2019

class MuiTooltip-tooltipPlacementBottom is always applied by default.
if props placement is set to top then MuiTooltip-tooltipPlacementTop is always applied.
It's not upated according to x-placement anymore

@oliviertassinari oliviertassinari added the status: waiting for author Issue with insufficient information label Sep 4, 2019
@capsule5 capsule5 changed the title tooltipPlacementBottom is always applied tooltipPlacement is not updated Sep 4, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 4, 2019

You can use the [x-placement*="top"] CSS selector as a workaround.

@oliviertassinari oliviertassinari added component: tooltip This is the name of the generic UI component, not the React module! support: question Community support but can be turned into an improvement and removed status: waiting for author Issue with insufficient information labels Sep 4, 2019
@capsule5
Copy link
Author

capsule5 commented Sep 4, 2019

thanks but does it mean that tooltipPlacementBottom/Top is not working anymore?
I was doing something like :

<Tooltip
    placement="top"
    classes={ {
      tooltip: styles[`tooltip--${theme}`],
      tooltipPlacementBottom: styles["tooltip--bottom"],

And it was working as expected: top by default and the extra MuiTooltip-tooltipPlacementBottom was applied when the tooltip placement was at bottom

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work and removed support: question Community support but can be turned into an improvement labels Sep 4, 2019
@oliviertassinari
Copy link
Member

@capsule5 Interesting, ignore my previous comment. This seems to be a regression coming from #16613.

@capsule5
Copy link
Author

capsule5 commented Sep 4, 2019

thanks your tip will do the job waiting for the fix

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 4, 2019

@capsule5 What do you think of this diff?

diff --git a/packages/material-ui/src/Popper/Popper.js b/packages/material-ui/src/Popper/Popper.js
index 55a4ff0903..5d6bd6c39b 100644
--- a/packages/material-ui/src/Popper/Popper.js
+++ b/packages/material-ui/src/Popper/Popper.js
@@ -77,14 +77,9 @@ const Popper = React.forwardRef(function Popper(props, ref) {
    * modifiers.flip is essentially a flip for controlled/uncontrolled behavior
    */
   const [placement, setPlacement] = React.useState(rtlPlacement);
-  if (rtlPlacement !== placement) {
-    setPlacement(rtlPlacement);
-  }

   const handleOpen = React.useCallback(() => {
-    const popperNode = tooltipRef.current;
-
-    if (!popperNode || !anchorEl || !open) {
+    if (!tooltipRef.current || !anchorEl || !open) {
       return;
     }

@@ -97,7 +92,7 @@ const Popper = React.forwardRef(function Popper(props, ref) {
       setPlacement(data.placement);
     };

-    const popper = new PopperJS(getAnchorEl(anchorEl), popperNode, {
+    const popper = new PopperJS(getAnchorEl(anchorEl), tooltipRef.current, {
       placement: rtlPlacement,
       ...popperOptions,
       modifiers: {
@@ -114,6 +109,7 @@ const Popper = React.forwardRef(function Popper(props, ref) {
       },
       // We could have been using a custom modifier like react-popper is doing.
       // But it seems this is the best public API for this use case.
+      onCreate: createChainedFunction(handlePopperUpdate, popperOptions.onCreate),
       onUpdate: createChainedFunction(handlePopperUpdate, popperOptions.onUpdate),
     });
     handlePopperRefRef.current(popper);

Do you want to submit a pull request with a fix :)? I'm not sure we can add any great test for it 🤔

cc @eps1lon for double-checking. It seems that the problem originates from the rtlPlacement !== placement branch that negates any effect of the on update logic. This can be particularity noticed by adding a console log in the popper render function and moving the anchor element to an edge, the console log should spam the console at each small scroll change on the edge. Still, I think that the other changes of #16613 are great to keep.

@oliviertassinari oliviertassinari added component: Popper The React component. See <Popup> for the latest version. and removed component: tooltip This is the name of the generic UI component, not the React module! labels Sep 4, 2019
@oliviertassinari oliviertassinari changed the title tooltipPlacement is not updated [Popper] Render function placement argument is not updated Sep 4, 2019
@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Sep 4, 2019
@joshwooding joshwooding added the hacktoberfest https://hacktoberfest.digitalocean.com/ label Sep 30, 2019
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: Popper The React component. See <Popup> for the latest version. good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants