-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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): restore support for dropups #1752
fix(dropdown): restore support for dropups #1752
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from a couple of typos
src/dropdown/dropdown.spec.ts
Outdated
@@ -70,8 +63,8 @@ describe('ngb-dropdown', () => { | |||
expect(compiled).not.toBeShown(); | |||
}); | |||
|
|||
it('should be up if up input is true', () => { | |||
const html = `<div ngbDropdown [up]="true"></div>`; | |||
it('should have dropup CSS class is placed on top', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't really understand what you mean. is → if ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, fixed.
src/dropdown/dropdown.ts
Outdated
@@ -37,7 +39,11 @@ export class NgbDropdownMenu { | |||
} | |||
}) | |||
export class NgbDropdownToggle { | |||
constructor(@Inject(forwardRef(() => NgbDropdown)) public dropdown, private _elementRef: ElementRef) {} | |||
anachorEl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo → anchor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -83,16 +86,26 @@ export class NgbDropdown { | |||
@Input('open') _open = false; | |||
|
|||
/** | |||
* Placement of a dropdown. Use "top-right" for dropups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I think you should either list the acceptable values in the doc or type it appropriately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defintivelly. We are going to add appropriate type in a PR that introduces more positioning options (including auto
).
Fixes ng-bootstrap#1747 Part of ng-bootstrap#1171 Part of ng-bootstrap#1012 BREAKING CHANGE: The `up` input is no longer supported by you can use more flexible `placement` setting now. Before: ```html <div ngbDropdown [up]="true"> ``` After: ```html <div ngbDropdown placement="top-right"> ``` Closes ng-bootstrap#1752
Fixes #1747
Part of #1171
Part of #1012
BREAKING CHANGE:
The
up
input is no longer supported by you can use more flexibleplacement
setting now.Before:
After: