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

Switch to Popper.js for tooltips/dropdowns positioning? #10544

Closed
FezVrasta opened this issue Aug 16, 2017 · 14 comments
Closed

Switch to Popper.js for tooltips/dropdowns positioning? #10544

FezVrasta opened this issue Aug 16, 2017 · 14 comments

Comments

@FezVrasta
Copy link

FezVrasta commented Aug 16, 2017

I see you have a lot of logic to handle the tooltips and popovers position, but still the result is far from optimal.
For instance, I see the tooltips won't flip to stay into viewport area if they overflow it.

Have you considered the idea to drop all your custom logic and rely on something like Popper.js?

Bootstrap v4 makes use of it and everything works nicely.

Project page: https://popper.js.org
Repo: https://github.com/FezVrasta/popper.js

@IamManchanda
Copy link
Contributor

IamManchanda commented Aug 16, 2017

Ah we like to use our own code for this type of stuff

See:
Thread => #9989
Inspiration => #9799
PR => #10053

@IamManchanda
Copy link
Contributor

Have you considered the idea to drop all your custom logic and rely on something like Popper.js?

cc @kball but if you ask me I dont think so...

@IamManchanda
Copy link
Contributor

Btw just saw that you are the creator of the popper framework... So what are the benefits of Popper over current JS code of ours @FezVrasta ??

@FezVrasta
Copy link
Author

FezVrasta commented Aug 16, 2017

I already provided a little explanation on the initial post. I'm going to write a comprensive list tomorrow if it's not a problem for you. It's a bit late here right now

@IamManchanda
Copy link
Contributor

IamManchanda commented Aug 17, 2017

No issue i will be waiting!

@FezVrasta
Copy link
Author

FezVrasta commented Aug 17, 2017

So, one of the problems I see is that the popovers/dropdowns/tooltips (poppers) don't attempt to appear (and stay) in the viewport, as you can see from this gif, they just appear in a non visible portion of the page.

1

I don't exactly know what's happening here, but looks like your algorithm have some problems detecting the correct place where to place the dropdowns:

2

Form my tests, I see that your algorithm doesn't support scrolling containers:
3

And last but not least, the PR that introduced the smart positioning in foundation added ~1500 rows of code you have to test and maintain.

Popper.js is ~2000 rows, included huge JSDocs comments, and has a code coverage of ~90% with lot of automated tests. (1038 without comments)

I think a switch to it would save you a lot of hassle and would provide a better experience to your users.

@IamManchanda
Copy link
Contributor

Thanks a lot @FezVrasta ... This satisfies me ...
Althought we like to keep our dependency very low and short but me and even @kball (had a talk for the same today morning) are very open about it...

But will need other people thoughts also
cc @brettsmason @SassNinja @rafibomb @paxperscientiam

@SassNinja
Copy link
Contributor

I agree to the downsides @FezVrasta has described regarding the current plugin.
The reposition doesn't seem to work 100% reliable to me for all scenarios, it doesn't get reset if possible (I've already made a PR for this) and it doesn't consider scrolling outside the viewport while being opened.

So in my eyes it's not wrong to use a well-tested positioning engine instead of a custom solution if it can be integrated into the current code without breaking backwards compatibility (or at least without the need to change a lot).

@IamManchanda
Copy link
Contributor

Yeah there is nothing bad in having a single positioning library instead of bunch of custom implementations that never reach a mature state.

We have to agree that not many tests/work have gone through our explicit positioning and it wont even happen coz foundation need to think broad and not just positioning
If there is something good as popper that can be used as positioning library then why not!

@kball
Copy link
Contributor

kball commented Aug 17, 2017

Would happily accept a PR for this, but as @SassNinja points out backwards compatibility is important.

@IamManchanda
Copy link
Contributor

@FezVrasta Would you like to do a PR for this ?

@kball I would say dont target this on 6.5 but 7 (Nov Release)

@FezVrasta
Copy link
Author

I don't know if I'll have time in these weeks. If I manage to finish my work I'll see if I'm able to work on this.

@IamManchanda
Copy link
Contributor

I don't think there is any rush if its targeted for F7...

@IamManchanda
Copy link
Contributor

V7 requests are being closed for the time being. We have them labeled so we can readdress them in the future.

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