Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

New positioning service #326

Closed
wants to merge 1 commit into from
Closed

New positioning service #326

wants to merge 1 commit into from

Conversation

pkozlowski-opensource
Copy link
Member

@angular-ui/bootstrap I was so fed up with the positioning issues we are having for tooltips, popovers and typeahead that I've created a new $position service that allows us to calculate coordinate of elements in relation to other elements. Have a look at the code.

There are 2 commits in this PR: the service itself and its usage in the typeahead directive.

The only problem is testing of this DOM positioning craziness. I've created a test page that aids with visual testing but really not sure how to create unit tests for this. It should be possible if we would base it on comparing position of 2 elements... but it would mean attaching all the elements to the document - this would slow down tests and I'm not sure how reliable it can be.

@joshdmiller I think that we can fix all the open tickets for tooltips / popovers with this new service so it would be awesome if you could have a look at it.

@drozzy
Copy link

drozzy commented Apr 15, 2013

Wow. Nice. Can we expect 0.3 soon then :-) ?

@joshdmiller
Copy link
Contributor

Hey @pkozlowski-opensource - I was thinking about developing something very similar to this, so awesome! I should have a chance to dive into this on Tuesday and give some feedback.

@ajoslin
Copy link
Contributor

ajoslin commented Apr 16, 2013

Nice pawel! Took a brief look, couldn't find any problems. I'm sure @joshdmiller has a better grasp on the popup stuff though :-)

@maxisam
Copy link

maxisam commented Apr 16, 2013

Can't wait to see 0.3 out ! Thanks for your hard work on this ! With this update, I can get rid of some ugly walk arounds !

@joshdmiller
Copy link
Contributor

@pkozlowski-opensource Sorry, I didn't get to this until today. :-(

I just created a simple Plunker where I migrated the $tooltip to the $position service, but it's not solving the positioning issue when in a relatively positioned parent: http://plnkr.co/edit/RHr9xNZ5Uj6ChubYjcx7?p=preview. I'm sure I'm just missing something terribly obvious...

I'll dive into this further Thursday or Friday.

@pkozlowski-opensource
Copy link
Member Author

@joshdmiller sorry, I thought that I will have time to look at this today but something else got in a way. Hopefully you will be able to look into this one, it must be some simple issue here. If not I will have a bit of time during the weekend.

@pkozlowski-opensource
Copy link
Member Author

@joshdmiller OK, tracked it down to this line:
ab0bce4#L0R18

It turns out that the .css() method returns only CSS properties defined as part of the style property and not computed, runtime CSS properties. We need to find a cross-browser way of computing runtime CSS properties ('position' to be precise).

@pkozlowski-opensource
Copy link
Member Author

@joshdmiller I think I've nailed it with the recent changes in 386374b. It required a change where the runtime CSS properties are retrieved. I've also went through a pain of making this work under IE8 - truly horrible experience...

Anyway, a plunk with a fix seems to be working OK:
http://plnkr.co/edit/FmFeUUcTAvIST9InBxN0?p=preview

Would be awesome if you guys could review it so I can merge it and add typeahead positioning fixes as well.

@joshdmiller For the tooltip fixes I would avoid calling $position.offset to get width/height of a tooltip element as this method does much more than this. Would simply do:

          ttPosition = {
              width: tooltip.prop( 'offsetWidth' ),
              height: tooltip.prop( 'offsetHeight' )
            };

Plunk has those modifications.

@bekos
Copy link
Contributor

bekos commented Apr 20, 2013

@pkozlowski-opensource I think your plunker points to previous commit.

@pkozlowski-opensource
Copy link
Member Author

@bekos Thnx for catching this, linked a wrong plunk, here is the correct one:
http://plnkr.co/edit/FmFeUUcTAvIST9InBxN0?p=preview

@pkozlowski-opensource
Copy link
Member Author

Landed as 77d9fa7

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

Successfully merging this pull request may close these issues.

6 participants