Skip to content
This repository was archived by the owner on Oct 2, 2019. It is now read-only.

Add an append-to-body attribute to <ui-select> #736

Merged
merged 5 commits into from
Mar 10, 2015

Conversation

cmlenz
Copy link
Contributor

@cmlenz cmlenz commented Mar 9, 2015

Add an append-to-body attribute to the <ui-select> directive that moves the dropdown element to the end of the body element before opening it, thereby solving problems with the dropdown being displayed below elements that follow the <ui-select> element in the document. This implementation is modeled after the typeahead-append-to-body support from UI Bootstrap, but adds the whole select element to the body, not just the dropdown menu, which is needed for the Select2 theme. See #41 (and quite a few dupes).

This replaces my previous PR #718 which did not work with the Select2 theme.

… moves the dropdown element to the end of the body element before opening it, thereby solving problems with the dropdown being displayed below elements that follow the `<ui-select>` element in the document. This implementation is modeled after the `typeahead-append-to-body` support from UI Bootstrap, but adds the whole select element to the body, not just the dropdown menu, which is needed for the Select2 theme. See angular-ui#41 (and quite a few dupes).
@cmlenz
Copy link
Contributor Author

cmlenz commented Mar 9, 2015

Oops, this is not quite ready yet. I've already noticed layout/positioning glitches in more complex layouts. Looking into it.

@dimirc
Copy link
Contributor

dimirc commented Mar 9, 2015

@cmlenz what kind of gitches? maybe you could paste a screenshot of what you found so we can try also similar escenarios to detect possible problems before merge. If you have the chance to add 1 or 2 tests that will be great also.

@brianfeister
Copy link

Just let us know when you're ready @cmlenz ... Also, @dimirc, thanks for chiming in, hopefully both of us can QA it before merging.

@cmlenz
Copy link
Contributor Author

cmlenz commented Mar 9, 2015

@dimirc The layout problem was easy to fix, I had forgotten to set the width on the select element. Otherwise it's working well for me. But this little directive has a surprising number of configurations that need to be tested :P

I understand that tests would be great ;) I'm looking at the existing test suite and will hopefully be able to figure out how to best add some.

@cmlenz
Copy link
Contributor Author

cmlenz commented Mar 9, 2015

Oh, should appendToBody also be added to the uiSelectConfig?

@brianfeister
Copy link

I think yes.

@cmlenz
Copy link
Contributor Author

cmlenz commented Mar 9, 2015

Okay, the layout issue is fixed, I've added some basic tests, and added appendToBody to the uiSelectConfig, too. Please test/review. Thanks!

@dimirc
Copy link
Contributor

dimirc commented Mar 9, 2015

LGMT, @brianfeister as you were reviewing this earlier, let us know if you have any comments else we could merge it

@brianfeister
Copy link

Sorry for how much effort this was @cmlenz, indeed, there are lots of edge cases with this little directive ;) ... you can see why we need tests.

Thanks so much, great work! 👍

brianfeister pushed a commit that referenced this pull request Mar 10, 2015
Add an append-to-body attribute to <ui-select>
@brianfeister brianfeister merged commit 55ae88b into angular-ui:master Mar 10, 2015
@cmlenz cmlenz deleted the feat-append-to-body2 branch March 10, 2015 15:30
@dimirc
Copy link
Contributor

dimirc commented Mar 11, 2015

If I don't use the append-to-body on /examples/demo-multi-select.html, the dropdown is presented partially behind of other component instance, which wasn't the case before this PR:

image

@cmlenz
Copy link
Contributor Author

cmlenz commented Mar 11, 2015

@dimirc Oops, you're right, I'm looking into this.

@cmlenz
Copy link
Contributor Author

cmlenz commented Mar 11, 2015

#745 should fix that.

@dimirc
Copy link
Contributor

dimirc commented Mar 12, 2015

@cmlenz thanks.

I see other similar issue also, could you check it?

image

@dmccown1500
Copy link

Just wanted to reach out and say thanks for getting this merged in and all the hard work on adding this. it is a lifesaver for my team! Thanks everybody!

@cmlenz
Copy link
Contributor Author

cmlenz commented Mar 12, 2015

@dimirc Which of the examples is that?

FYI I've seen a couple issues with the appendToBody support after merging the most recent changes. Looking into those right now.

@dmccown1500
Copy link

Yeah it looks like the build did not bring append to body into the dist

@jarrodpayne
Copy link

I'm a little late to the party here, but should append to body be the default behavior for the dropdown? This is essentially how the native select dropdown works. Also, it seems that the behavior would be beneficial for every app. Maybe I'm missing something. Please advise.

@dmccown1500
Copy link

I would think it would be a little unexpected to switch it to a default. and it is easy enough to make it the default for your app by using the uiSelectConfig option

@dmccown1500
Copy link

Looking through the commit messages it seems like it is intended to
On Mar 12, 2015 6:10 PM, "lebster" [email protected] wrote:

does appendToBody work on the global config?


Reply to this email directly or view it on GitHub
#736 (comment).

@lebster
Copy link

lebster commented Mar 13, 2015

sorry, the issue was not when selecting the value (i had the wrong css).

But It does have an issue with setting with widths.

http://plnkr.co/edit/RcW0Wln1TfekvZJpkf9m?p=preview

@cmlenz
Copy link
Contributor Author

cmlenz commented Mar 13, 2015

@lebster That should be fixed by 47b55d1

I've updated your Plunk with the most recent version:

http://plnkr.co/edit/d5ZGqxHDZpTNf6NX726C?p=preview

@dimirc
Copy link
Contributor

dimirc commented Mar 15, 2015

@cmlenz the problem is present on /examples/demo.html

Seems related to z-index between bootstrap and select2 themes

@cmlenz
Copy link
Contributor Author

cmlenz commented Mar 16, 2015

@dimirc That was supposed to be fixed by #745

That PR was merged but somehow got lost(?!), it is not in master. Any idea what happened there?

@dimirc
Copy link
Contributor

dimirc commented Mar 16, 2015

@cmlenz don't know what happened but I push new commit with those changes 66bf8b5

@jarrodpayne
Copy link

@dmccown1500 Yeah, your right. But, my suggestion is not about it being difficult to make default for my app. I suggest it as the default / only behavior because this is simply how the dropdown should behave. If the append to body functionality was a first class citizen contributors wouldn't need to be concerned with both approaches when, as far as I'm concerned, this approach is "the" way to go. It mimics native, does the same job, and solves the problem of overflow divs etc.

If anything, I would suggest this as default with the ability to specify another parent element if necessary much like how angular material handles modal dialogs.

@sohel-ahmed-ansari
Copy link

Hmm. Seems to work. But if I my ui-select dropdown is in a scrollable div and I scroll the div while the dropdown is open, the dropdown seems to remain in the same position. Attaching a screenshot for better understanding.
screenshot from 2015-04-24 19 22 14

@brianfeister
Copy link

If in a modal, you should be disabling the append-to-body feature

@longedok
Copy link

Just wanted to mention that I have the same problem as @sohel-ahmed-ansari, though my whole page is inside a scrollable area, and I can't avoid using append-to-body, as my select is also inside a table row with hidden overflow.
selection_141

@sohel-ahmed-ansari
Copy link

@brianfeister : Actually I was using the append-to-body feature because my dropdown is inside a modal.
So what happens is if the dropdown is at the bottom of the modal it gets cut, because my overflow-y is set to auto. Now adding append-to-body solves my problem of dropdown getting cut, but then if I scroll it creates my above mentioned problem.

@brianfeister
Copy link

@sohel-ahmed-ansari & @longedok - please feel free to submit a pull request if you'd like to fix this issue. I've dealt with it on other projects and it's quite a bit of work to solve. Agreed that it's a problem. If it were me I would disable append-to-body and make changes to my modal with some kind of overflow: visible so that the ui-select element dropdown lives inside of the modal's DOM element.

@sohel-ahmed-ansari
Copy link

@brianfeister : Yeah I know its a lot of work. Was just wondering if there was any quick work around. I will provide a pull request if I come up with a good solution. For now I have downgraded to ui-select2 as all the cases I want are handled in it. The reason I had upgraded to ui-select was themes. But now I will override ui-select2's css for my required theme. Appreciate your time you gave to this. Thanks.

@btm1
Copy link

btm1 commented Mar 18, 2016

I'm confused I see that there is a append-to-body attribute in the directive but as far as I can tell it does nothing... nothing is being appended to the body and everything is still inline. Where are we at on this?

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.

9 participants