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

Docs: use <button> for dropdown/popover examples, explain preference over <a> for dropdowns, tweak collapse prose #37432

Merged
merged 4 commits into from
Nov 12, 2022

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Nov 5, 2022

Description

  • Expand dropdowns prose to explain that <button> is recommended/preferred. Change subsequent example to use <button>
  • Remove unnecessary role="button" from <button> elements in collapse unit tests
  • Tweak modal examples to use <button> for popovers inside modal, and for the modal trigger itself
  • Tweak the prose/explanation for the collapse component

Motivation & Context

Closes #37426

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

@julien-deramond
Copy link
Member

The changes LGTM and make this part of the documentation clearer.

However I'm wondering it we shouldn't also modify the following things:

@patrickhlauke
Copy link
Member Author

@julien-deramond sorry, wasn't quite finished with this. was just too tired last night, but wanted to at least make a start - hence the draft ;)

@julien-deramond
Copy link
Member

@julien-deramond sorry, wasn't quite finished with this. was just too tired last night, but wanted to at least make a start - hence the draft ;)

My bad. Hadn't seen the draft 😇

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Nov 5, 2022

the dropdowns with .nav-link are currently stylistically problematic, yes.

taking https://getbootstrap.com/docs/5.2/components/navbar/#supported-content as an example, changing the control to <button class="btn btn-link nav-link dropdown-toggle">...</button> gets us close, but the vertical text alignment is off

screenshot of the navbar, with the dropdown using a button, with the text in the button not aligning with the other links

that is, i think, one of the main reasons why for some components/patterns, authors still use an <a href="..." role="button"> at the moment...because the alternative just doesn't work out of the box.

I think that's something to consider for v6 ... reviewing all the styles to make it possible to seamlessly mix and match links and real <button> elements and have them line up vertically correctly

EDIT: filed #37433

@patrickhlauke patrickhlauke changed the title Expand dropdown explanation for <a>, use <button>s for modal examples Docs: use <button> for dropdown/popover examples, explain preference over <a> for dropdowns Nov 5, 2022
@patrickhlauke patrickhlauke changed the title Docs: use <button> for dropdown/popover examples, explain preference over <a> for dropdowns Docs: use <button> for dropdown/popover examples, explain preference over <a> for dropdowns, tweak collapse prose Nov 5, 2022
@patrickhlauke patrickhlauke marked this pull request as ready for review November 5, 2022 16:27
@patrickhlauke patrickhlauke requested a review from a team as a code owner November 5, 2022 16:27
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done!

@mdo mdo merged commit 0446e22 into main Nov 12, 2022
@mdo mdo deleted the patrickhlauke-issue37426 branch November 12, 2022 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Enhance dropdown link examples
4 participants