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

Proposed patch for issue #11 #13

Merged
merged 2 commits into from
Oct 8, 2017

Conversation

coderwassananmol
Copy link
Contributor

Patch for #11

@StanleySathler
Copy link
Member

Hello @coderwassananmol
Great commit, bro!

Can I give you a better approach?

As explained here, the problem occurs because the part of the code which calculates the content's height is not considering the presence of the filter.

Check the file _popup.js, line 170. There, you can see that the content's height is calculated based on popup's height and the title's height.

If the select has a filter (remember: there is a parameter saying that, default is false), we should consider the filter too. The solution would be something like that:

let popupHeight = _calculatePopupHeight();
let titleHeight = _$title.outerHeight();
let filterHeight = _$filter.outerHeight();

let contentHeight = (popupHeight - titleHeight);
if(_properties.hasFilter)
	contentHeight = contentHeight - filterHeight;

_$content.outerHeight(contentHeight);

You must check if the hasFilter is true because even when the filter field is not visible, it has the size 44px. It's because its visibility is controlled by display: none and not something like height: 0

@coderwassananmol
Copy link
Contributor Author

Brilliant @StanleySathler
I have updated the patch. I hope this works!

@StanleySathler StanleySathler merged commit 5b3b6c2 into ss-javascript:master Oct 8, 2017
@StanleySathler
Copy link
Member

Yay! Very excited with the contribution, @coderwassananmol. Already merged. 🔥

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.

2 participants