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.

Sign up for GitHub

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

Jump to bottom

Refactor tests for dropdown menu #2114

Merged
merged 6 commits into from
Apr 10, 2024
Merged

Refactor tests for dropdown menu #2114

merged 6 commits into from
Apr 10, 2024

Conversation

jelhan
Copy link
Contributor

@jelhan jelhan commented Feb 29, 2024 •
edited

Dropdown is aligned based on CSS class if used within a <BsNav> or <BsNavbar>. Bootstrap 5 expects the .dropdown-menu-end CSS class for the alignment. But Ember Bootstrap rendered the .dropdown-menu-right CSS class for Bootstrap 5 as well. That one is only correct for Bootstrap 4.

Additionally this fixes a bug rendering .dropdown-menu-false CSS class if @align argument was not set. That was introduced in 94ed264#diff-6edad28cc2b90ce452b543d7e652aea3319372185fdc0b84ebad39a812820252L46-R48 but didn't had any visual impact.

@jelhan jelhan added the bug label Feb 29, 2024
@jelhan jelhan marked this pull request as draft March 1, 2024 18:32
@jelhan jelhan changed the title [WIP] Dropdown menu should be left aligned by default Fix right alignment of Dropdown within nav and navbar Mar 1, 2024
@jelhan jelhan changed the title Fix right alignment of Dropdown within nav and navbar Fix right alignment of Dropdown within nav and navbar for BS5 Mar 1, 2024
@jelhan jelhan marked this pull request as ready for review March 1, 2024 20:00
@jelhan jelhan changed the title Fix right alignment of Dropdown within nav and navbar for BS5 Fix right alignment of dropdown menu within nav and navbar for BS5 Mar 1, 2024
SanderKnauff
SanderKnauff requested changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@SanderKnauff SanderKnauff left a comment

Choose a reason for hiding this comment

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

Looks good! Just a minor nit comment, so if you fix that it's ready to be merged.

test-app/tests/integration/components/bs-dropdown/menu-test.js Outdated Show resolved Hide resolved
ember-bootstrap/addon/components/bs-dropdown/menu.js Outdated Show resolved Hide resolved
@jelhan jelhan mentioned this pull request Apr 10, 2024
Open
@jelhan
Copy link
Contributor Author

jelhan commented Apr 10, 2024

@SanderKnauff: I think this was addressing the same problem as #2120. I'm sorry that I caused you doing the same work just by not following up on this one. Honestly speaking I only noticed when resolving merge conflicts that it was about the same issue.

This one is no limited to refactoring the tests. Would be great if you could review anyways. I think it is still a small improvement.

@jelhan jelhan changed the title Fix right alignment of dropdown menu within nav and navbar for BS5 Refactor tests for dropdown menu Apr 10, 2024
@jelhan jelhan added internal and removed bug labels Apr 10, 2024
@SanderKnauff
Copy link
Contributor

@SanderKnauff: I think this was addressing the same problem as #2120. I'm sorry that I caused you doing the same work just by not following up on this one. Honestly speaking I only noticed when resolving merge conflicts that it was about the same issue.

This one is no limited to refactoring the tests. Would be great if you could review anyways. I think it is still a small improvement.

No worries! I also completely forgot about this one. The changes look good, so this can be merged!

@jelhan jelhan merged commit 5d810ab into master Apr 10, 2024
22 checks passed
@jelhan jelhan deleted the dropdown-menu-wrong-default-alignment branch April 10, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SanderKnauff SanderKnauff SanderKnauff approved these changes

Assignees
No one assigned
Labels
internal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jelhan @SanderKnauff