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

Add 'auto' as placement option #3009

Merged

Conversation

patrikholcak
Copy link
Contributor

About

As discussed in #2583, adding autoPlacement is not possible due to flip being automatically added by shepherd. This PR re-introduces auto placements, which were removed a couple of versions back with migration from popper to floating-ui.

When step attachTo.on is set to auto[-start|end], we automatically add autoPlacement to the middlewares and remove flip, which isn’t compatible with it.

Proposed changes

  • add auto[-start|end] placement options, which were removed when popper was replaced with floating-ui
    type PopperPlacement = 'auto'|'auto-start'|'auto-end'|'top'|'top-start'|'top-end'|'bottom'|'bottom-start'|'bottom-end'|'right'|'right-start'|'right-end'|'left'|'left-start'|'left-end';

Let me know if there’s any changes you want me to make 🙏

Copy link

vercel bot commented Oct 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
shepherd-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 18, 2024 0:50am
shepherd-landing ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 18, 2024 0:50am

Copy link

vercel bot commented Oct 11, 2024

@patrikholcak is attempting to deploy a commit to the shipshapecode Team on Vercel.

A member of the Team first needs to authorize it.

@RobbieTheWagner
Copy link
Member

Thanks for the PR @patrikholcak! Do we need to add these new options to the docs?

@patrikholcak
Copy link
Contributor Author

I’m not sure! Looks like it’s correctly added to the type in docs. Let me know if you want me to add a recipe, though not sure if that’s needed

@RobbieTheWagner
Copy link
Member

@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!

@patrikholcak
Copy link
Contributor Author

patrikholcak commented Oct 18, 2024

@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 shift middleware. From my testing it still can produce weird positions due to the limiter: limitShift(), option. Maybe the best solution is to have an option to disable the default middlewares altogether (#3010).

@RobbieTheWagner
Copy link
Member

@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 shift middleware. From my testing it still can produce weird positions due to the limiter: limitShift(), option. Maybe the best solution is to have an option to disable the default middlewares altogether (#3010).

@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 auto option to work. Can you explain what you mean by weird positions? I want to make this as seamless for users as possible.

@patrikholcak
Copy link
Contributor Author

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 shift middleware, so I can add padding from the sides (maybe that fixed it)

Anyway, this should be it. :shipit: 🙏

@RobbieTheWagner RobbieTheWagner merged commit bce520c into shipshapecode:main Oct 25, 2024
7 of 8 checks passed
@github-actions github-actions bot mentioned this pull request Oct 25, 2024
@patrikholcak patrikholcak deleted the feat/add-autoPlacement branch October 27, 2024 06:45
@ulken
Copy link
Contributor

ulken commented Nov 19, 2024

@RobbieTheWagner when can I expect this to be released?

@RobbieTheWagner
Copy link
Member

@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.

@github-actions github-actions bot mentioned this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants