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 guard rail to dropdown keyhandler #37755

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Dec 29, 2022

Description

Adds a guard rail/sanity check to the keyboard handler, to bail out if there's no actual toggle button

Motivation & Context

While good practice in general (as it does not assume there is a toggle button), this also prevents the type error in our documentation where we have examples of dropdown menus that are "static" and already opened.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

https://deploy-preview-37755--twbs-bootstrap.netlify.app/docs/5.3/components/dropdowns/#active

Related issues

Closes #37428

@GeoSot
Copy link
Member

GeoSot commented Dec 31, 2022

As much as I want to protect us of future misleading issues related to docs examples, I would never prefer to add this dummy "silencer" as it will affect the real implementations' strictness and feedback. And for sure, a dropdown without a toggler, has wrong markup 😞

Maybe is a better, just add a note on dropdown examples

@patrickhlauke
Copy link
Member Author

x-ref #34349

@GeoSot GeoSot force-pushed the patrickhlauke-issue37428 branch from e91f966 to cedd335 Compare February 3, 2023 21:47
@patrickhlauke
Copy link
Member Author

any decision/thoughts on this?

@patrickhlauke
Copy link
Member Author

@GeoSot @julien-deramond so what do we want to do here? I think the options are to:

  • tweak the documentation to somehow silence/special case the open dropdowns used as examples (and maybe be more explicit in the documentation about our dropdown menu actually needing to be in a menu), or
  • add this guard rail here (and maybe still do the documentation tweak above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Escape keydown gets TypeError on Dropdown elements
3 participants