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(dropdown): wrong positioning calculation #3058

Merged
merged 1 commit into from
Mar 28, 2019

Conversation

fbasso
Copy link
Member

@fbasso fbasso commented Mar 8, 2019

fix #3056

The fix only adds the 'show' class before the positioning, but I don't know if it's the best solution as the class attribute is bound. I tried to 'unbind' the class, but it's much more painfull.

@codecov-io
Copy link

codecov-io commented Mar 8, 2019

Codecov Report

Merging #3058 into master will decrease coverage by 1.44%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3058      +/-   ##
==========================================
- Coverage   92.27%   90.83%   -1.45%     
==========================================
  Files          91       90       -1     
  Lines        3015     2999      -16     
  Branches      485      484       -1     
==========================================
- Hits         2782     2724      -58     
- Misses        171      207      +36     
- Partials       62       68       +6
Flag Coverage Δ
#e2e 100% <ø> (+39.3%) ⬆️
#unit 90.83% <100%> (+0.06%) ⬆️
Impacted Files Coverage Δ
src/dropdown/dropdown.ts 92.9% <100%> (-0.51%) ⬇️
src/util/focus-trap.ts 45% <0%> (-55%) ⬇️
src/util/autoclose.ts 29.16% <0%> (-50%) ⬇️
src/util/accessibility/live.ts 85.18% <0%> (-11.12%) ⬇️
src/typeahead/typeahead.ts 92.05% <0%> (-5.3%) ⬇️
src/popover/popover.ts 97.5% <0%> (-2.5%) ⬇️
src/util/positioning.ts 91.93% <0%> (-2.42%) ⬇️
src/datepicker/datepicker-input.ts 95.65% <0%> (-1.45%) ⬇️
src/tooltip/tooltip.ts 97.1% <0%> (-1.45%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5444d13...b5f8a07. Read the comment docs.

@fbasso fbasso force-pushed the positioning-dropdown branch 3 times, most recently from e368fb9 to de3fda9 Compare March 14, 2019 15:10
@fbasso fbasso force-pushed the positioning-dropdown branch 2 times, most recently from c9afe6f to 273aa85 Compare March 18, 2019 09:08
@maxokorokov maxokorokov added this to the 4.1.1 milestone Mar 25, 2019
Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

LGTM!

Could you pease just make the test description clearer ?

const dropdownElement = document.querySelector('div[ngbDropdownMenu]');
const parentContainer = dropdownElement.parentNode;
expect(parentContainer).toHaveCssClass('dropdown');
expect(parentContainer.parentNode).toBe(document.body, 'The dropdown should be attached to the body');

});

it('should toggle at the second placement if needed', () => {
Copy link
Member

Choose a reason for hiding this comment

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

should second placement if the first one doesn't fit ?

fixture.detectChanges();
const dropdownEl = compiled.querySelector('[ngbdropdownmenu]');
const targetElement = compiled.querySelector('button');
// expect(dropdownElement.getAttribute('x-placement')).toBe('right');
Copy link
Member

Choose a reason for hiding this comment

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

please remove or uncomment

src/dropdown/dropdown.ts Show resolved Hide resolved
Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@maxokorokov maxokorokov merged commit e9f7b9e into ng-bootstrap:master Mar 28, 2019
@fbasso fbasso deleted the positioning-dropdown branch April 5, 2019 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown uses only the first specified placement.
4 participants