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

feat(v5): Update dropdown #5471

Merged
merged 2 commits into from
Oct 26, 2020
Merged

feat(v5): Update dropdown #5471

merged 2 commits into from
Oct 26, 2020

Conversation

kyletsang
Copy link
Member

https://v5.getbootstrap.com/docs/5.0/components/dropdowns/

Added the dark dropdown variant initially.

Changes:

  • Set default element of DropdownDivider to hr
  • Add variant prop for DropdownMenu
  • Add menuVariant prop for DropdownButton and NavDropdown

I noticed that Bootstrap changed their dropdown menu/item markup back to using ul/li in twbs/bootstrap#28591 . We should probably do the same here. I wanted to get some feedback before adding those changes though. In DropdownItem, I could make it so the props control the li element, then apply the anchor specific props/classes to the inner anchor... but then this probably need additional props to control rendering of the anchor. Alternatively, add a bare li wrapping element and keep the current implementation as is?

@kyletsang kyletsang added this to In Progress in v5 support via automation Oct 9, 2020
@kyletsang kyletsang marked this pull request as draft October 9, 2020 20:26
- Set default element of DropdownDivider to hr
- Add variant prop for DropdownMenu
- Add menuVariant prop for DropdownButton and NavDropdown
@kyletsang kyletsang marked this pull request as ready for review October 9, 2020 21:37
Copy link
Member

@jquense jquense left a comment

Choose a reason for hiding this comment

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

looks great.

re: 'li' tags, i think it'd be hard to default to them given how flexible menus are intended to be. I'd be worried that folks would accidentally great more bad markup. We should probably do the right thing in doc examples tho via as maybe?

@kyletsang
Copy link
Member Author

kyletsang commented Oct 13, 2020

Won't be able to use as here since we still need the anchor to render within the li. I think I vaguely recall that the class in BS3 used to be on the li element, but looks like they opted to place it in the children instead for BS5.

<li><a class="dropdown-item" href="#">Action</a></li>

Would it make sense to add as="li" as a special case for DropdownItem, and have that render the above code?

Scratch suggestion. Would need to apply this to dropdown item text, divider, header as well.

@kyletsang
Copy link
Member Author

Going to kick the li can down the road since the styling works with the current markup.

@kyletsang kyletsang merged commit f728394 into bs5-dev Oct 26, 2020
v5 support automation moved this from In Progress to Done Oct 26, 2020
@kyletsang kyletsang deleted the v5-dropdown branch October 26, 2020 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v5 support
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants