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

Fix alignment of dropdown menus when using Bootstrap 5 #2120

Merged

Conversation

SanderKnauff
Copy link
Contributor

Bootstrap 5 uses start/end instead of left/right. The menu was still using the value from the align argument. In addition to this, the styles used in Bootstrap 5 check for the existence of the [data-bs-popper] attribute. This has been added to the menu as well.

I noticed this issue while converting the app at work from BS4 to BS5.

Before fix:
image

After fix:
image

Bootstrap 5 uses start/end instead of left/right. The menu was still using the value from the align argument.
In addition to this, the styles used in Bootstrap 5 check for the existence of the [data-bs-popper] attribute. This has been added to the menu as well
Copy link
Contributor

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

I was too quick with my first review. Only noticed failing CI afterwards. And that led me to some change requests.


switch (this.align) {
case 'left':
return 'dropdown-menu-start';
Copy link
Contributor

Choose a reason for hiding this comment

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

I double checked in Bootstrap docs for v5. If I get it right, .dropdown-menu-start is not needed as that's the default: https://getbootstrap.com/docs/5.3/components/dropdowns/#menu-alignment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I had not noticed that. I will remove the left/start

);
assert
.dom('.dropdown-menu')
.hasClass(alignmentClass, `menu has ${alignmentClass} class`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do t expect it to have a class in BS4 and 5.

I think we should assert that it does not have any class except .dropdown-menu. Not sure how well that's supported by qunit-dom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

QUnit DOM may not, but I'll do the assertions on the classlist.

@jelhan jelhan added the bug label Apr 10, 2024
@SanderKnauff SanderKnauff merged commit 10d87a3 into ember-bootstrap:master Apr 10, 2024
22 checks passed
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.

None yet

2 participants