Add md-autocomplete-snap="width" #7750

aperl · 2016-03-25T16:28:09Z

This allows a md-autocomplete-wrap to be applied to an ancestor of a md-autocomplete and cause that element to be used for evaluating the width of the autocomplete instead of the the md-autocomplete-wrap element. This addresses #4450 which constrains the width of the menu that pops up to the width of the auto complete wrapper.

When using chips and possibly in other situations it would be desirable to have the menu be the width of the chips component.

Adding support for this attribute would allow for this and other similar behavior.

A working version of this change can be seen at https://codepen.io/allenperl/pen/KzmQwr.

googlebot · 2016-03-25T16:28:10Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2016-03-25T16:28:10Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

devversion · 2016-03-25T16:34:55Z

src/components/autocomplete/js/autocompleteController.js

@@ -207,6 +207,17 @@ function MdAutocompleteCtrl ($scope, $element, $mdUtil, $mdConstant, $mdTheming,
 elements.$ = getAngularElements(elements);
 }

+ /**
+ * Finds the correct element that will determine the width


That's not a valid description. getWrapTarget will be not used to only determine the width. As the function says, it's just scanning for the wrap target.

The warp is used to calculate the width of the menu. I"ll update the description accordingly.

devversion · 2016-03-25T16:37:10Z

@aperl Thanks for the PR, but please provide more details in your Pull Request description. For example, what's the use-cases etc.

googlebot · 2016-03-25T17:15:29Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-03-25T17:15:29Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

devversion · 2016-03-25T17:18:25Z

src/components/autocomplete/js/autocompleteController.js

@@ -208,6 +208,17 @@ function MdAutocompleteCtrl ($scope, $element, $mdUtil, $mdConstant, $mdTheming,
 }

 /**
+ * Find the target for the wrap element which determines menu width


I'm still not happy with the function description. The wrap is not specializing on determining the menu width.

While the name wrap describes what the element is, as far as the controller is concerned it only uses the wrap to calculate the width which can be found in positionDropdown (). I could change the comment to something like "Find the target for the wrap element." But I thought more description help convey why this code is there. Again the description is similar to that of getSnapTarget(). In the end I'll make the description to whatever would be satisfactory.

aperl · 2016-03-25T17:24:19Z

@devversion I've updated the initial comment to add use case and further description. I'll look now for the appropriate place to add this to documentation.

googlebot · 2016-03-25T17:50:52Z

CLAs look good, thanks!

googlebot · 2016-03-25T17:50:52Z

CLAs look good, thanks!

topherfangio · 2016-03-25T18:30:02Z

I like the principle of this PR, but it definitely needs some tests. I am rerunning the Travis build to see if it passes existing tests; something threw off the original build.

aperl · 2016-04-06T18:29:59Z

@topherfangio, what sort of test would we be looking for?

topherfangio · 2016-04-06T18:49:09Z

Regarding tests; I think it could be relatively simple. Just test both cases:

When not using a wrapper, the autocomplete's width is the same as the md-autocomplete component's width
When using a wrapper, it should be the same as the wrapper

topherfangio · 2016-04-06T18:59:35Z

src/components/autocomplete/js/autocompleteController.js

+ * @returns {*}
+ */
+ function getWrapTarget() {
+ for (var element = $element; element.length; element = element.parent()) {


I'm wondering if there is a better way to accomplish this using a directive. Perhaps the md-autocomplete-wrap-override could be a full directive that searches for child autocomplete's (perhaps using some combination of .querySelector() and .controller()) and register with them as the wrapper so that we don't have to traverse the entire tree of parents all the way back up the DOM.

@aperl You think you could give this a shot?

@topherfangio It wouldn't be that hard. I"ll hop on it.

@topherfangio So the md-autocomplete-wrap-override seemed like an easy way to do it but then it got fragile when I looked at a situation where the md-autocomplete element resided inside and ng-if. In this case the md-autocomplete-wrap-override can't find the md-autocomplete controller to register to.

I suspect it is for similar reasons md-autocomplete-snap is found by walking up the DOM.

Hmm, good point; okay, let me think about it a bit more. Please ping me early next week if I forget to respond though 😄

Another way to go would be to piggy back on the md-autocomplete-snap attribute and give it an optional value md-autocomplete-snap="width". This would avoid adding another attribute and then we could only have to walk up the DOM once.

That's not a bad idea; although I am a bit confused as to how this doesn't already affect the width of the item. Are positioning and width somehow separate?

(I didn't actually know md-autocomplete-snap existed since it's not in the docs)

For whatever reason the md-autocomplete-snap was only made to effect the vertical position.

Okay, if it's relatively easy to do; why don't you go ahead with the md-autocomplete-snap="width" approach. That will give us the most flexibility.

If you can also add some docs to the Autocomplete about it's usage, that would be great!

I'll update the PR

googlebot · 2016-04-13T20:24:04Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-04-13T20:24:04Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2016-04-13T20:36:36Z

CLAs look good, thanks!

googlebot · 2016-04-13T20:36:37Z

CLAs look good, thanks!

aperl · 2016-04-13T20:47:40Z

@topherfangio where would I go to add documentation about md-autocomplete-wrap.

topherfangio · 2016-04-14T15:39:51Z

@aperl I would just add it to the existing autocomplete docs. They can be found at the top of autocompleteDirective.js and I would probably add a short paragraph right before the "Validation" section.

aperl · 2016-04-18T17:48:52Z

@topherfangio the documentation changes are in.

aperl · 2016-10-25T18:26:45Z

@topherfangio the conflicts were simple. The PR has been rebased.

topherfangio · 2016-10-25T18:44:03Z

@aperl Thanks so much for the super-fast turn around!

aperl · 2016-10-25T18:44:58Z

@topherfangio is there anything else I need to do?

topherfangio · 2016-10-25T18:49:35Z

@aperl Nope. It's labelled as presubmit, so it will go through Google's presubmit process where they run it against internal tests. If all the tests pass, then it should get immediately merged. If they fail, we should get some more info on the PR with the failure so we can investigate.

If it happens to break a TON of tests, it's possible that it may get rejected (with a note), but since this is a small addition/non-breaking change, I wouldn't expect that.

aperl · 2016-10-25T18:52:20Z

@topherfangio thanks for the info

jelbourn · 2016-12-02T23:31:46Z

@aperl can you rebase?

aperl · 2016-12-05T16:58:24Z

@jelbourn Rebase done.

barryvdh · 2016-12-23T12:46:13Z

This seems to work very well like this:

 <md-chips md-autocomplete-snap="width" style="width: 100%">
            <md-autocomplete ..

But without both the ="width" and the style it doesn't work, is this intentional? I would have guessed md-autocomplete-snap would just make the chips full width already. But perhaps I'm not understanding correctly.

We're hoping to build a 1.1.2 next week, so if we can get this done this week, that would be great!

Would be nice to have this in a release indeed :)

aperl · 2016-12-23T15:27:31Z

@barryvdh md-autocomplete-snap was already a feature, though perhaps not well documented. It would cause the dropdown to snap the the specified element. For reverse compatibly it was decided to add the ="width" value to force it to snap to the width as well.

aperl mentioned this pull request Mar 25, 2016

Autocomplete dropdown is too narrow when inside of md-chips #4450

Closed

devversion reviewed Mar 25, 2016
View reviewed changes

devversion added needs: work needs: review This PR is waiting on review from the team labels Mar 25, 2016

devversion reviewed Mar 25, 2016
View reviewed changes

aperl force-pushed the fix/autocomplete branch from 0d8772c to 3cd2281 Compare March 25, 2016 17:50

aperl changed the title ~~Add xw-autocomplete-wrap-override~~ Add md-autocomplete-wrap-override Apr 4, 2016

topherfangio reviewed Apr 6, 2016
View reviewed changes

aperl force-pushed the fix/autocomplete branch from 56d2399 to efe6f4d Compare April 13, 2016 20:36

topherfangio added needs: presubmit and removed needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved labels Oct 25, 2016

ThomasBurleson modified the milestones: 1.1.2, - Backlog Nov 29, 2016

aperl added 6 commits December 5, 2016 09:46

add xw-autocomplete-wrap-override

c01da71

fix white space and description

48bcd26

create md-autocomplete-warp="width"

6ae2aed

add md-autocomplete-snap documentation

6ac0e3d

move if statement

44c30d6

add md-autocomplete-snap tests

741df73

aperl force-pushed the fix/autocomplete branch from 7891060 to 741df73 Compare December 5, 2016 16:57

googlebot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ labels Dec 5, 2016

fix merge error

64f68b3

mmalerba merged commit 1e45c44 into angular:master Dec 6, 2016

aperl deleted the fix/autocomplete branch December 23, 2016 15:28

Splaktar modified the milestones: - Backlog, 1.1.2 Jun 28, 2018

OleksandrBudik mentioned this pull request Jul 27, 2018

Chips and full-width autocomplete angular/components#12401

Closed

Add md-autocomplete-snap="width" #7750

Add md-autocomplete-snap="width" #7750

Conversation

aperl commented Mar 25, 2016

googlebot commented Mar 25, 2016

googlebot commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devversion commented Mar 25, 2016

googlebot commented Mar 25, 2016

googlebot commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aperl commented Mar 25, 2016

googlebot commented Mar 25, 2016

googlebot commented Mar 25, 2016

topherfangio commented Mar 25, 2016

aperl commented Apr 6, 2016

topherfangio commented Apr 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Apr 13, 2016

googlebot commented Apr 13, 2016

googlebot commented Apr 13, 2016

googlebot commented Apr 13, 2016

aperl commented Apr 13, 2016

topherfangio commented Apr 14, 2016

aperl commented Apr 18, 2016

aperl commented Oct 25, 2016

topherfangio commented Oct 25, 2016

aperl commented Oct 25, 2016

topherfangio commented Oct 25, 2016

aperl commented Oct 25, 2016

jelbourn commented Dec 2, 2016

aperl commented Dec 5, 2016

barryvdh commented Dec 23, 2016

aperl commented Dec 23, 2016 • edited Loading

aperl commented Dec 23, 2016 •

edited

Loading