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 possibility to use another container for suggestion items list (example: document.body) #17086

Open
wants to merge 14 commits into
base: gh-pages
Choose a base branch
from

Conversation

bsvobodny
Copy link

add possibility to use another container for suggestion items list (example: document.body)
Indentation fixed ;)

@LeaVerou
Copy link
Owner

Please check the diffs you are submitting before you ask for review. It looks like there is still changed indentation.
Also, please do not pollute the commit history. If I merge this, I’ll have 4+ commits added to the commit history. It does not warrant more than 1, the rest are commits that just fix issues with the first one.
Also, I noticed you changed the version number in package.json. Please don't do that. It’s not up to any individual PR to decide when we are going to do a new release.
Thank you.

@bsvobodny
Copy link
Author

ok, let me fix all that, and I will try again.

@bsvobodny
Copy link
Author

I fixed all the indentation issues and rollback the version in package.json

The last issue is the commit history, but I don't know how can I merge all my commits in one.

@bsvobodny
Copy link
Author

It seems that I managed to fix all the indent issues and merge all my commits in one.

I hope it's fine now ;)

thanks

Copy link
Owner

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick response! Just left some feedback on the actual changes, now that I can finally review them without all the unintentional changes getting in the way.

Zooming out a bit, what is unclear to me is, what use cases does this solve? E.g. what was your use case that made you need this functionality?

awesomplete.js Outdated
@@ -169,7 +171,11 @@ _.prototype = {
},

open: function () {
if (this.listContainer !== 'container') {
this.setListPosition();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be _.CONTAINER?

awesomplete.js Outdated
@@ -179,6 +185,14 @@ _.prototype = {
$.fire(this.input, "awesomplete-open");
},

setListPosition: function () {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to only be calling this function from one place, so it seems better to inline it there with a comment about what this code does. If in the future it's needed to reposition this list from multiple places then it can be moved to a function.

awesomplete.js Outdated
@@ -179,6 +185,14 @@ _.prototype = {
$.fire(this.input, "awesomplete-open");
},

setListPosition: function () {
//get the bounding of the container to set the position of the suggestion list below
var boudingRect = this.container.getBoundingClientRect()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bounding, you missed a "n"

package.json Outdated
@@ -29,4 +29,4 @@
"karma-jasmine": "^0.3.6",
"karma-jasmine-def": "^0.1.0"
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for any changes here?

@LeaVerou
Copy link
Owner

@vlazar want to add any feedback?

@bsvobodny
Copy link
Author

@LeaVerou the reason why I needed this feature is because I'm using an autocomplete field in an area where the overflow is hidden, and the size of the area is limited so the suggestion list cannot go out of this area and is not visible.

@bsvobodny
Copy link
Author

@LeaVerou I made some change accordingly to your comments.

@rustam87
Copy link

this changes are entered or no? because i am not seeing in production code

@jack829
Copy link

jack829 commented Apr 26, 2018

@LeaVerou Any chance this will happen (soon)? I have the same issue as @bsvobodny.

melitele pushed a commit to melitele/awesomplete that referenced this pull request May 27, 2018
addresses a use case where container element cannot reuse input element parent
because input parent clips container content

also solves LeaVerou#17086
melitele pushed a commit to melitele/awesomplete that referenced this pull request May 27, 2018
addresses a use case where container element cannot reuse input element parent
because input parent clips container content

also solves LeaVerou#17086
melitele pushed a commit to melitele/awesomplete that referenced this pull request May 27, 2018
addresses a use case where container element cannot reuse input element parent
because input parent clips container content

also solves LeaVerou#17086 & LeaVerou#16718
melitele pushed a commit to melitele/awesomplete that referenced this pull request May 30, 2018
addresses a use case where container element cannot reuse input element parent
because input parent clips container content

also solves LeaVerou#17086 & LeaVerou#16718
LeaVerou pushed a commit that referenced this pull request May 30, 2018
I've added a container function to options that will allow client to provide a different container element. The default version of container function creates container element as before. I believe this is in line with the item function that customizes creating item elements.

This addresses my use case where container element cannot reuse the input element parent
because the input parent clips container's content. It also addresses a case when input element has to be next to it's original siblings.

It also solves #17086 & #16718

Thank you for consideration.
@Goldjan
Copy link

Goldjan commented Jun 25, 2018

Is it now possible to specify the body as a container?
Something like this: container: 'document.body'

@lucax88x
Copy link

lucax88x commented Jul 4, 2018

@LeaVerou I love the library, I try to use it every time I can, but every time I always encounters problem when I use it on a modal, or something that has an overflow, please merge this feature.. or implement it

@LeaVerou
Copy link
Owner

LeaVerou commented Jul 4, 2018

I'm ok merging this if the conflicts are resolved!

@lucax88x
Copy link

lucax88x commented Jul 5, 2018

I noticed there's a new Container property in the options, it's on the docs but not released.

I tried it myself copying from github, it works partially... I can add the div to the body, but it's not correctly positioned and does not have the correct 'width', it's this intended for something else?

@bsvobodny
Copy link
Author

It seems that the new container could do the job, I didn't try it yet.

@lucax88x
Copy link

lucax88x commented Jul 5, 2018

As I said, it's probably not complete.. maybe we can 'finish' the current container? position & width are not working with this property

@LeaVerou
Copy link
Owner

LeaVerou commented Jul 6, 2018

@bsvobodny Thanks for resolving the conflicts! Can you try if the container property does what you need and let me know? It's probably not a good idea to merge a PR that duplicates existing functionality (if that's the case) and unfortunately I don't have the time to check it properly myself right now. :(

@LeaVerou
Copy link
Owner

LeaVerou commented Jul 6, 2018

Also it looks like Travis is complaining that several tests are failing. Have you run npm test before submitting this?

@bsvobodny
Copy link
Author

I did it after... :(

@DRocksCoding
Copy link

Hello I searched through the issues to find some tips on this situation for myself too.

TLDR: how do you actually use the container property? I don't understand the explanation and didn't see a example. I'd jsut like to sit the ul inside the body.

I am trying to instantiate a awesomplete inside a table td cell but I need the ul element to sit outside of the table otherwise it gets hidden in the table overflow. I've tried messing around a few hours with the css and theres no success in my current setup.

Does any one have more experience with this than myself and could give some tips please ?
Thanks in advance

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.

7 participants