-
-
Notifications
You must be signed in to change notification settings - Fork 650
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
Add 'auto' as placement option #3009
Add 'auto' as placement option #3009
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@patrikholcak is attempting to deploy a commit to the shipshapecode Team on Vercel. A member of the Team first needs to authorize it. |
Thanks for the PR @patrikholcak! Do we need to add these new options to the docs? |
@patrikholcak I think we also mention it in https://docs.shepherdjs.dev/guides/usage/ and perhaps other places. Just want to make sure we update it all! |
@RobbieTheWagner, checked the code and looks like the usage page is the only other place which has a list of positions, so that should (probably?) be it! One thing I’m debating here is whether to also disable the |
@patrikholcak awesome, thank you! I think ideally we make things "just work" for folks. We have updated the order of merging for middleware, so if you want to override any middleware, whatever you pass in floatingUIOptions will win. That being said, I don't think people should need to override the middleware to get this new |
I agree with the idea of making things "just work." However, sometimes — when using libraries — you hit a brick wall simply because you can’t override certain defaults that were put in place to make things simpler (or ensure a degree of backwards compatibility in this case). In those cases, I think those should definitely be overridable. That said, I don’t think everything needs to be customisable — but I digress. That’s a discussion that PR. What I meant by weird positioning is: sometimes when the target element is in a corner, the tour step can get clip outside of the window. But I am not able to replicate it now 🤷 And yes, looks like I am able to override the default Anyway, this should be it. 🙏 |
@RobbieTheWagner when can I expect this to be released? |
@ulken sorry I did not realize we had not released it. I just pressed the button, so hopefully it'll be out here in a sec. |
About
As discussed in #2583, adding
autoPlacement
is not possible due toflip
being automatically added by shepherd. This PR re-introducesauto
placements, which were removed a couple of versions back with migration from popper to floating-ui.When step
attachTo.on
is set toauto[-start|end]
, we automatically addautoPlacement
to the middlewares and removeflip
, which isn’t compatible with it.Proposed changes
auto[-start|end]
placement options, which were removed when popper was replaced with floating-uishepherd/src/types/step.d.ts
Line 232 in 76fe594
Let me know if there’s any changes you want me to make 🙏