muicss / mui

Lightweight CSS framework
https://www.muicss.com
Other
4.51k stars 426 forks source link

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

Open englishextra opened 6 years ago

englishextra commented 6 years ago

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 https://github.com/ryanve/verge/issues/19 to get the picture what's going on.

Thanks.

amorey commented 6 years ago

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

englishextra commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@englishextra Here's a pull request that should fix this issue: https://github.com/muicss/mui/pull/277

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

amorey commented 6 years ago

@englishextra Have you had a chance to look at pull request https://github.com/muicss/mui/pull/277?

englishextra commented 6 years ago

@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.