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

Can't use FloutingUI autoPlacement #2583

Closed
sirmspencer opened this issue Jan 23, 2024 · 10 comments
Closed

Can't use FloutingUI autoPlacement #2583

sirmspencer opened this issue Jan 23, 2024 · 10 comments

Comments

@sirmspencer
Copy link

Shepherd adds flip and shift to default options, then does a deep merge with any other options passed in. Flip and autoPlacement are not compatible so adding autoPlacement to floatingUIOptions.middleware just causes errors.

I tried not passing in a placement so shouldCenter is true, but there are side effects in other parts of the code that block floating UI all together.

You could check for something like :on "auto" to skip adding flip. autoPlacement doesn't need a placement so you can skip options.placement = attachToOptions.on; too.

https://github.com/shepherd-pro/shepherd/blob/master/src/js/utils/floating-ui.js#L169

@patrikholcak
Copy link
Contributor

Same here, version 12.0.1, if I pass on: 'auto' the step is positioned weirdly.

Popper.js Floating UI
Screenshot 2024-05-14 at 12 04 46 Screenshot 2024-05-14 at 12 04 49

Another problem is with passing offset middleware, when clicking between steps, the step jumps to its offset. Not sure if tied to the same problem

Screen.Recording.2024-05-14.at.12.10.52.mp4

@chuckcarpenter
Copy link
Member

@patrikholcak 'auto' isn't an option for position, we just use the as noted by FUI

export type PopperPlacement =
  | 'top'
  | 'top-start'
  | 'top-end'
  | 'bottom'
  | 'bottom-start'
  | 'bottom-end'
  | 'right'
  | 'right-start'
  | 'right-end'
  | 'left'
  | 'left-start'
  | 'left-end';

Sounds like what is asked by @sirmspencer is an option to have 'auto' and then use autoPlacement middleware and omit flip.

@patrikholcak
Copy link
Contributor

Right, so should I open another issue for my problem? I can’t upgrade because of this. 😞

@sirmspencer
Copy link
Author

@patrikholcak 'auto' isn't an option for position, we just use the as noted by FUI

export type PopperPlacement =
  | 'top'
  | 'top-start'
  | 'top-end'
  | 'bottom'
  | 'bottom-start'
  | 'bottom-end'
  | 'right'
  | 'right-start'
  | 'right-end'
  | 'left'
  | 'left-start'
  | 'left-end';

Sounds like what is asked by @sirmspencer is an option to have 'auto' and then use autoPlacement middleware and omit flip.

I think so. Its been 6 months so id have to look a little deeper again.

@chuckcarpenter
Copy link
Member

@patrikholcak @sirmspencer we'd be happy to also take a PR to prioritize this if you're up for it?

@RobbieTheWagner
Copy link
Member

It seems like you both had a good handle on what you wanted and where to insert it into the library, so a PR would be great if you have the time!

@RobbieTheWagner
Copy link
Member

@sirmspencer @patrikholcak we changed the ordering of the merging for the floatingUIOptions. Could you please see if things work better for you now?

@patrikholcak
Copy link
Contributor

👋 I just opened a PR for auto positions support #3009

@sirmspencer
Copy link
Author

I'll take a look, thanks!

@RobbieTheWagner
Copy link
Member

Closed by #3009

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants