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

dropdown.js " - wrapperRect.top" is useless #275

Open
englishextra opened this issue Apr 22, 2018 · 7 comments
Open

dropdown.js " - wrapperRect.top" is useless #275

englishextra opened this issue Apr 22, 2018 · 7 comments

Comments

@englishextra
Copy link

englishextra commented Apr 22, 2018

While UL with menu items stays display none it has no top value at all which is 0.
It gets a value other than 0 only if the UL is revealed - display block for instance.

So:

  // method to open dropdown
  function openDropdownFn() {
    // position menu element below toggle button
    var wrapperRect = wrapperEl.getBoundingClientRect(),
        toggleRect = toggleEl.getBoundingClientRect();

    var top = toggleRect.top - wrapperRect.top + toggleRect.height;

is the same as:

  // method to open dropdown
  function openDropdownFn() {
    // position menu element below toggle button
    var wrapperRect = wrapperEl.getBoundingClientRect(),
        toggleRect = toggleEl.getBoundingClientRect();

    var top = toggleRect.top - 0 + toggleRect.height;

See this issue here ryanve/verge#19 to get the picture what's going on.

Thanks.

@amorey
Copy link
Member

amorey commented Apr 23, 2018

Thanks for the heads up. Does this cause a problem with the Dropdown component or is it just unnecessary code?

@englishextra
Copy link
Author

englishextra commented Apr 23, 2018

@amorey this is what I do:

var handleDropdownButton = function (evt) {
	evt.stopPropagation();
	evt.preventDefault();
	var _this = this;
	var dropdownMenu = _this.nextElementSibling;
	var dropdownButtonRect = _this.getBoundingClientRect();
	var top = dropdownButtonRect.top + dropdownButtonRect.height;
	var left = dropdownButtonRect.left;
	if (dropdownMenu) {
		dropdownMenu[style].top = top + "px";
		if (!dropdownMenu[classList].contains("mui-dropdown__menu--right")) {
			dropdownMenu[style].left = left + "px";
		}

full code here

demo

@amorey
Copy link
Member

amorey commented Apr 24, 2018

Hi @englishextra - what do you mean by wrapperRect.top being equal to 0? The value of wrapperRect.top should be the distance from the top of the wrapper element to the top of the viewport.

In any case, taking a closer look at the code I think we can remove the references to toggleRect.top and wrapperRect.top and instead use the outer height of the toggle element:

var top = toggleEl.offsetHeight;

Let me know if there's something I'm not understanding with this issue.

@englishextra
Copy link
Author

@amorey wrapperRect.top is not 0 only after the UL reveals that is gets display block.

Perhaps yes as it's said in verge.js issue

if (verge.inViewport(e) && 0 !== e.offsetHeight) {

Well the code that I presented works fine for me. So you decide whichever is appropriate.

amorey added a commit that referenced this issue Apr 24, 2018
@amorey
Copy link
Member

amorey commented Apr 24, 2018

@englishextra Here's a pull request that should fix this issue: #277

Let me know if you notice any problems with the new code.

@amorey
Copy link
Member

amorey commented Apr 28, 2018

@englishextra Have you had a chance to look at pull request #277?

@englishextra
Copy link
Author

@amorey Hi. Yes, As I understand you have solved the issue completely in a new mananer. But sorry I haven't studied the code closely. I will very soon. Thanks for your quick responses.

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

No branches or pull requests

2 participants