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

Dropdown menu alignment fixes #32986

Merged
merged 3 commits into from
Feb 9, 2021
Merged

Dropdown menu alignment fixes #32986

merged 3 commits into from
Feb 9, 2021

Conversation

mdo
Copy link
Member

@mdo mdo commented Feb 5, 2021

This does a couple things in an attempt to resolve, at least temporarily, our dropdown menu alignment issues.

  1. Removes the &[style] { ... } override @MartijnCuppens added. This alone resolves Dropdown menu not positioned correctly with .dropstart #32484, but this caused other issues with the menu alignment classes on static dropdowns.

  2. So, to fix that, now the menu alignment classes .dropdown-menu-start and .dropdown-menu-end are positioned via Popper and use the --bs-position CSS variable. I've removed the left and right properties for those.

  3. And, in combination, I've scoped those left and right properties to instances of the -start and -end classes that are used with data-bs-display="static". It's not the best, but it resolves the alignment near as I can tell for all dropdown menus, static or otherwise.

In the docs, I've added a temporary set of all the dropdown alignment options at the top of the page so I can quickly check things. I'll migrate this to the cheatsheet pages I think before merging, but didn't want to do that until getting some eyes on this.

Whatcha think—good enough for solving our woes ahead of Beta 2?

/cc @ffoodd @XhmikosR

TODO:

  • clean up squash any patches manually
  • needs tests

@mdo mdo requested a review from a team as a code owner February 5, 2021 02:18
@mdo mdo added this to Inbox in v5.0.0-beta2 via automation Feb 5, 2021
@rohit2sharma95
Copy link
Collaborator

This would restrict the user to use the the display static option only via data attributes. User can create the static dropdown via JavaScript as well.

@mdo
Copy link
Member Author

mdo commented Feb 5, 2021

This would restrict the user to use the the display static option only via data attributes. User can create the static dropdown via JavaScript as well.

@rohit2sharma95 Gah, forgot about that. Any ideas for how we could do something programmatically in our JS to better handle this then?

@mdo mdo requested a review from a team as a code owner February 5, 2021 17:50
@rohit2sharma95
Copy link
Collaborator

This is now stable fix IMO 🙂
Let me know your thoughts @mdo

@XhmikosR XhmikosR moved this from Inbox to Review in v5.0.0-beta2 Feb 5, 2021
@XhmikosR
Copy link
Member

XhmikosR commented Feb 5, 2021

@rohit2sharma95 can you add some tests please? Also, can you explain why we the last patch of yours?

@rohit2sharma95
Copy link
Collaborator

Because these data attributes are being added just for the styling of the dropdown menu, so instead of adding the attributes on the button element it would be good to add them on the dropdown menu directly.

@rohit2sharma95
Copy link
Collaborator

Since now there is a data attribute available to be sure about the usage of the Popper in the dropdown, the following code can be changed by using data-bs-popper:

&:not([data-popper-placement]) {
margin-top: $dropdown-spacer;
}

@mdo
Copy link
Member Author

mdo commented Feb 7, 2021

Awesome, if this looks good to y'all @XhmikosR and @rohit2sharma95, we can merge and then review #33003 and possibly merge that before Beta 2. Fine to wait until Beta 3 though if that's better!

site/content/docs/5.0/migration.md Outdated Show resolved Hide resolved
v5.0.0-beta2 automation moved this from Review to Approved Feb 8, 2021
@ffoodd
Copy link
Member

ffoodd commented Feb 8, 2021

There's something missing for RTL, as you can see in this PR RTL Cheatsheet: not quite sure about what to do however.

@mdo
Copy link
Member Author

mdo commented Feb 8, 2021

@ffoodd Do you mean we're missing an example in the RTL cheatsheet, or we're missing a bug?

@rohit2sharma95
Copy link
Collaborator

Popper cannot bear the right style on the popper elements 😄

@ffoodd
Copy link
Member

ffoodd commented Feb 8, 2021

Oh sorry @mdo, yeah it's buggy in RTL since dropdown menu it's not right aligned until playing around with devtools.

@rohit2sharma95
Copy link
Collaborator

Now seems fixed @ffoodd
https://deploy-preview-32986--twbs-bootstrap.netlify.app/docs/5.0/examples/cheatsheet-rtl/#dropdowns

@ffoodd
Copy link
Member

ffoodd commented Feb 9, 2021

Indeed, thanks @rohit2sharma95! 🚀

- Removes the &[style] selector that was used for resetting Popper styles
- Separate Popper-based alignment from static alignment with `data-bs-popover` attribute that separates the --bs-position and custom right/left properties

Co-Authored-By: Rohit Sharma <rohit2sharma95@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.0.0-beta2
  
Done
Development

Successfully merging this pull request may close these issues.

Dropdown menu not positioned correctly with .dropstart
4 participants