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

v5.2.1 dropdown TypeError #37111

Closed
3 tasks done
XhmikosR opened this issue Sep 9, 2022 · 4 comments · Fixed by #37190
Closed
3 tasks done

v5.2.1 dropdown TypeError #37111

XhmikosR opened this issue Sep 9, 2022 · 4 comments · Fixed by #37190
Labels

Comments

@XhmikosR
Copy link
Member

XhmikosR commented Sep 9, 2022

Prerequisites

Describe the issue

<div class="dropdown">
  <ul class="dropdown-menu">
    <li class="d-block d-lg-none">
      <h6 class="dropdown-header text-wrap">
        WHATEVER
      </h6>
    </li>
    <li>
      <hr class="dropdown-divider d-block d-md-none">
    </li>
    <li>
      <button class="dropdown-item" type="button">Logout</button>
    </li>
  </ul> <!-- .dropdown-menu -->
</div> <!-- .dropdown -->
Uncaught TypeError: this._menu is undefined
    _isShown bootstrap.bundle.js:4040
    toggle bootstrap.bundle.js:3910
    <anonymous> bootstrap.bundle.js:4241
    handler bootstrap.bundle.js:397
    invokeTask zone.js:406
    runTask zone.js:178
    invokeTask zone.js:487
    invokeTask zone.js:1661
    globalCallback zone.js:1704
    globalZoneAwareCaptureCallback zone.js:1729

which points to:

_isShown() {
  return this._menu.classList.contains(CLASS_NAME_SHOW$6);
}

Same markup works with 5.2.0.

Reduced test cases

See above

What operating system(s) are you seeing the problem on?

Windows, Android

What browser(s) are you seeing the problem on?

Chrome, Firefox

What version of Bootstrap are you using?

v5.2.1


Seems like 337068f is to blame

@GeoSot
Copy link
Member

GeoSot commented Sep 13, 2022

You are right on the 337068f, however according to docs the dropdowns are initialized on the .dropdown-toggle element (probably it would be better to initialized on parent, but this is another discussion)

As the .dropdown-toggle doesn't exist on the given markup, it is normal, to be initialized in a wrong way. So do you suggest keeping it as it is and support only the correct markup, or at least for 5v keep support for this too?

@GeoSot GeoSot moved this to Todo in v5.2.2 Sep 13, 2022
@XhmikosR
Copy link
Member Author

XhmikosR commented Sep 15, 2022

Ah, I remember now. So, the main issue is that I didn't want the dropdown toggle icon so I just went with a plain old button and data-bs-toggle="dropdown". Now, I'm pretty sure I can't be the only one doing this even though it's not 100% correct.

So,

  1. this was a breaking change, even though the usage isn't 100% right
  2. we could maybe have a dropdown-toggle without the icon
  3. or we could change the code to look for data-bs-toggle="dropdown" instead of the dropdown-toggle class

I feel like the last solution would be the simplest, but it's been a while I looked at the code.

@XhmikosR
Copy link
Member Author

XhmikosR commented Sep 15, 2022

Also, do note that we don't explicitly mention the dropdown-toggle class, but we do mention the data-bs-toggle="dropdown" attribute https://getbootstrap.com/docs/5.2/components/dropdowns/#usage

@mdo
Copy link
Member

mdo commented Sep 22, 2022

@XhmikosR @GeoSot I do like the idea of it trigger only off of the data attribute instead of the .dropdown-toggle class. I'm good with any fix here, but that's a good hygiene thing for us to double check for v6. I personally think all JS behavior should be controlled by data attribute, and styling limited to classes. Let's discuss more in Slack and find a path forward :).

@julien-deramond julien-deramond moved this from Todo to Needs review in v5.2.2 Sep 22, 2022
@julien-deramond julien-deramond moved this from Needs review to Review in progress in v5.2.2 Sep 23, 2022
@julien-deramond julien-deramond moved this from Review in progress to Ready to merge in v5.2.2 Sep 23, 2022
Repository owner moved this from Ready to merge to Done in v5.2.2 Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants