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

Setting order/hierarchy of modifiers #1094

Closed
nickofthyme opened this issue Apr 27, 2020 · 19 comments · Fixed by #1095
Closed

Setting order/hierarchy of modifiers #1094

nickofthyme opened this issue Apr 27, 2020 · 19 comments · Fixed by #1095
Labels
feature This would be nice to have.

Comments

@nickofthyme
Copy link

nickofthyme commented Apr 27, 2020

Feature description

Provide a way to set a hierarchy or order for modifiers. As far as I can tell looking through the docs, this is not possible and the order of the modifiers array is not respected.

This may only pertain to some modifiers as some are independent of others or don't perform conditional changes such as offset.

This could be added by respecting the array order or adding order prop on TModifier such as below.

createPopper(anchorRef, tooltipRef, {
  modifiers: [
    {
      name: 'preventOverflow',
      order: 1,
    },
    {
      name: 'flip',
      order: 2,
    },
  ],
});

It seems this logic would have to go in the core but I'm not sure if this could be implemented in the modifiers themselves.

Backstory

I am using popperjs to display a tooltip for a chart at a given cursor position which I am calculating, which is the blue element in the screenshots (aka the anchorRef).

It is working amazingly! However, I came across a case where I would like the preventOverflow modifier to take precedence over the flip modifier.

These are the options I am passing to createPopper

createPopper(anchorRef, tooltipRef, {
  strategy: 'fixed',
  placement: 'right',
  modifiers: [
    {
      name: 'offset',
      options: {
        offset: [0, 10],
      },
    },
    {
      name: 'preventOverflow',
      options: {
        tether: false,
        boundary: chartContainerRef,
      },
    },
    {
      name: 'flip',
      options: {
        fallbackPlacements: ['right', 'left', 'top', 'bottom'],
        boundary: chartContainerRef,
      },
    },
  ],
});

The issue is that I want bottom and top to be fallbackPlacements, which causes flip to set the placement to bottom even though preventOverflow would have positioned the tooltip inside the boundary. So this is what I actually get with the setup above.

Screen Recording 2020-04-27 at 10 45 AM

What I would like to see is this. Where at the top preventOverflow pushes the tooltip down to stay inside the boundary. And on the far right, flip changes placement to left and preventOverflow still pushes down at the top. So preventOverflow takes priority over flip, but in cases where a flip is required, it still performs the flip, such as on the far right.

Screen Recording 2020-04-27 at 10 40 AM

If there is a way to achieve the above without this feature, I am open to suggestions. Either way, I think this could be a good enhancement.

Why should this feature be part of the Popper's core?

  • Allows greater control and flexibility in using popperjs and modifiers
  • This feature seems to be a relatively small code change, afaik
@nickofthyme nickofthyme added feature This would be nice to have. NEEDS: triage labels Apr 27, 2020
@FezVrasta
Copy link
Member

Have you checked the requires property?

@nickofthyme
Copy link
Author

Good idea but same effect when using flip as

{
  name: 'flip',
  requires: ['preventOverflow'],
  options: {
    fallbackPlacements: ['right', 'left', 'top', 'bottom'],
  },
}

The requires option seems to be more of a functional dependency that is required to execute a given modifier. Whereas what I am proposing would be optional.

@FezVrasta
Copy link
Member

FezVrasta commented Apr 27, 2020

requiresIfExists should do the trick then?

@nickofthyme
Copy link
Author

Nope, I tried that as well. That would just be required dependencies that would influence the functionality of a given modifier if it existed. I am looking to control the order of the governing modifier, not necessarily when it was executed.

Somewhere in the code flip is implicitly taking precedence over preventOverflow. This is fine in most cases when the fallbackPlacements are somewhat restricted. But when fallbackPlacements are unrestricted flip will always control the positioning, rendering preventOverflow useless with the exception of maybe a few edge cases I can't think of right now.

@FezVrasta
Copy link
Member

AFAIK preventOverflow and flip are independent by each other, so you should be able to force one of them to run before the other by defining the requires property to point to the one you want to run as first.

Alternatively you may override the phase of one of them, so if you wanted flip to run as last, you could set phase to afterMain.

If this isn't helpful, I think I'll need a small repro to better understand what are you looking for.

@nickofthyme
Copy link
Author

I haven't looked at the code but I would assume it is somewhat iterative. And may be required to change this in the flip modifier.

Current flow

  • flip checks if placement on right is within the boundary
  • if NO, flip checks if placement on left is within the boundary
  • if NO, flip checks if placement on bottom is within the boundary
  • if YES, preventOverflow checks if placement bottom needs repositioning and if so can it fit on bottom
  • Done

Expected flow

  • flip checks if placement on right is within the boundary
  • if NO, preventOverflow checks if placement right needs repositioning and if so can it fit on right
  • if NO, flip checks if placement on left is within the boundary
  • if NO, preventOverflow checks if placement left needs repositioning and if so can it fit on left
  • if NO, flip checks if placement on bottom is within the boundary
  • if NO, preventOverflow checks if placement bottom needs repositioning and if so can it fit on bottom
  • Done

@nickofthyme
Copy link
Author

Yeah I played with the phases a little as you suggested and still getting the same. It seems like what I am asking for requires an interdependency between flip and preventOverflow.

@nickofthyme
Copy link
Author

Is the current flow I described above correct? Such that one modifier finishes executing before another takes over, provided all the required* dependencies are met? Where there is no back and forth between modifiers?

@FezVrasta
Copy link
Member

The flip modifier will reset the update cycle if it detects the preferred placement is not suitable. So the flow should look like the one you expect.

@nickofthyme
Copy link
Author

Ok good to know. But even though it resets the update cycle it appears to be executing flip before preventOverflow every cycle no matter the required modifiers.

Thanks for the information, I'll try to put up a repro in the next few days as well as take a look into how popper is executing the modifiers.

@FezVrasta
Copy link
Member

Honestly the behavior shown in your second clip should be the one provided by Popper out of the box, if that's not the case I'd consider that a bug. I'll wait for your repro so that we can verify it.

@FezVrasta FezVrasta added bug Something is not working. NEEDS: user action and removed feature This would be nice to have. NEEDS: triage labels Apr 27, 2020
@nickofthyme
Copy link
Author

The main difference between the two clips is the fallbackPlacements, out of the box they are set to ['right', 'left'] I assume, and loosening the restriction to ['right', 'left', 'top', 'bottom'] creates the behavior in the first clip.

Either way, I'll ping you when I have a repro.

@atomiks
Copy link
Collaborator

atomiks commented Apr 27, 2020

It's this #1011 - you don't want to check altAxis in this case

@nickofthyme
Copy link
Author

No combination of minAxis altAxis changes the functionality in my case, the placement always goes to the bottom instead of using preventOverflow.

@atomiks
Copy link
Collaborator

atomiks commented Apr 27, 2020

Yeah because the feature isn't implemented yet 😅 but it looks like there's a use case for it so we should add it

@nickofthyme
Copy link
Author

lol 🤣 sorry I missed that 😅. Yeah, that'd be spectacular!

@FezVrasta FezVrasta added feature This would be nice to have. and removed bug Something is not working. NEEDS: user action labels Apr 28, 2020
@FezVrasta
Copy link
Member

@nickofthyme May you confirm that PR addresses your problem please?

@nickofthyme
Copy link
Author

nickofthyme commented Apr 28, 2020

@FezVrasta with your latest changes, setting altAxis to false on the flip modifier now renders as expected!

Thanks for the quick response! When do you expect to publish 2.4.0?

@FezVrasta
Copy link
Member

now 😛

@GeoSot GeoSot mentioned this issue May 2, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This would be nice to have.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants