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 support for corner positions. #206

Merged
merged 4 commits into from
May 21, 2017
Merged

Add support for corner positions. #206

merged 4 commits into from
May 21, 2017

Conversation

leonard-thieu
Copy link
Contributor

Fixes #205

This PR adds support for corner positions (top left, top right, bottom left, bottom right). This also modifies the smart option to include the corner position as candidates. However, it appears the smart algorithm doesn't work correctly even in the most recent commit (44da315). I believe this also fixes a bug with onTooltipSideChange where it may have failed to remove the old attribute.

Also, I didn't know what to do with the tip arrows. They ended up looking pretty ugly.

@45kb
Copy link
Member

45kb commented May 20, 2017

@leonard-thieu tried it and looks working, but atm is not valid to be merged unfortunately.

  • :not() looks not widely supported http://caniuse.com/#search=%3Anot
  • The tip arrow is not showing up
    schermata 2017-05-20 alle 09 24 08
    this should show the tip arrow on top left i guess :)
  • Please do not commit dist/ folder

Can you make these changes?

thanks a lot for your time!

@leonard-thieu
Copy link
Contributor Author

:not() looks not widely supported http://caniuse.com/#search=%3Anot

That's for the selector list argument. I'm only using single simple selectors.

The tip arrow is not showing up

I'm not sure why the tip arrow is not showing up for you (or why the tooltip isn't positioned correctly). I can't reproduce it. This is how it displays for me:

corner-positions-browsers

@leonard-thieu
Copy link
Contributor Author

Also, I'm thinking I should change the order of the directions that the smart algorithm iterates through. Originally, smart selected counter-clockwise from the start direction (except for bottom for which it went clockwise). I tried to keep some backwards-compatibility by having it try cardinal directions first but I didn't implement it correctly (and it wouldn't work for bottom anyway).

I think it might make more sense to have it cycle through all directions in order (i.e. top -> top left -> left -> bottom left). This should result in it selecting a position closer to the start position and take less iterations to find a position.

@45kb
Copy link
Member

45kb commented May 20, 2017

@leonard-thieu

That's for the selector list argument. I'm only using single simple selectors.

You're right sorry i found the wrong :not()

I'm not sure why the tip arrow is not showing up for you (or why the tooltip isn't positioned correctly). I can't reproduce it.

Now it works also for me, i cloned your fork https://github.com/leonard-thieu/angular-tooltips and checked out the corner-positioins branch, everythings ok now :)

So let me know how you want to proceed for the directions order, if to open a new PR and merge this or if to wait a little, no problems ... :)

@leonard-thieu
Copy link
Contributor Author

I'll just make the change now since it hasn't been merged yet.

@leonard-thieu
Copy link
Contributor Author

This should be good to go.

@45kb
Copy link
Member

45kb commented May 21, 2017

schermata 2017-05-21 alle 08 52 35
@leonard-thieu tried it locally https://github.com/leonard-thieu/angular-tooltips/tree/corner-positions

but it doesn't seem to work, am i missing anything?

thank you.

@leonard-thieu
Copy link
Contributor Author

angularjs_tooltips_-_google_chrome_2017-05-21_08-26-23
I can't reproduce and I'm not sure why it's positioning top instead of top left for you.

@45kb 45kb merged commit 9fc8850 into 720kb:master May 21, 2017
@45kb
Copy link
Member

45kb commented May 21, 2017

@leonard-thieu sorry my bad this was the problem https://github.com/720kb/angular-tooltips/blob/master/index.html#L359 :) must be src/ not dist/ thanks for this great addition, i am going to release it

@45kb
Copy link
Member

45kb commented May 21, 2017

@leonard-thieu leonard-thieu deleted the corner-positions branch May 21, 2017 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support corner positions
2 participants