Bootstrap4 beta dropdowns #1745
Conversation
Closes #1022 BREAKING CHANGE: Dropdown menu now requires usage of the new `ngbDropdownMenu` directive. Before: ```html <div ngbDropdown> <button ngbDropdownToggle>Toggle dropdown</button> <div class="dropdown-menu"> <a class="dropdown-item">Action</a> </div> </div> ``` After (notice **ngbDropdownMenu**): ```html <div ngbDropdown> <button ngbDropdownToggle>Toggle dropdown</button> <div ngbDropdownMenu> <a class="dropdown-item">Action</a> </div> </div> ```
src/dropdown/dropdown.ts
Outdated
| @@ -26,7 +74,7 @@ export class NgbDropdown { | |||
| /** | |||
| * Indicates that dropdown should be closed when selecting one of dropdown items (click) or pressing ESC. | |||
| */ | |||
| @Input() autoClose: boolean; | |||
| @Input() autoClose: boolean | 'outside' | 'inside'; | |||
There was a problem hiding this comment.
Should update documentation for 2 new values
src/dropdown/dropdown.ts
Outdated
| export class NgbDropdownMenu { | ||
| isOpen = false; | ||
|
|
||
| constructor(private _elementRef: ElementRef) {} |
There was a problem hiding this comment.
Can't you inject the NgbDropdown as you do in the NgbDropDownToggle and remove internal isOpen state which is duplicated now?
Would it work if you just use [class.show]: 'dropdown.isOpen()'?
Can't see why you want to explicitly have open and show methods
There was a problem hiding this comment.
Great catch, it is much simpler indeed! Changing.
src/dropdown/dropdown.ts
Outdated
| } | ||
|
|
||
| toggleOpen() { this.dropdown.toggle(); } | ||
| private _isEventFromMenu($event) { | ||
| if (this._menu) { |
There was a problem hiding this comment.
can be a ternary one-liner, but don't know if you like it :)
There was a problem hiding this comment.
Yeh, changing to ternary
| beforeEach(() => { | ||
| jasmine.addMatchers(jasmineMatchers); |
There was a problem hiding this comment.
I think it could be done once in beforeAll
There was a problem hiding this comment.
Will open a separate issue / PR for this as I've noticed other things in this test that need attention.
I will open a separate issue for this as a proper fix will require usage of the positioning service and we need to design it properly. |
|
Thnx for the review @maxokorokov - great feedback. Waiting for Travis to get green and merging it. There are 2 more points needed attention:
|
|
Merged into the |
This builds on top of #1743 and adds Bootstrap 4.beta specific markup changes (the
showclass needs to be added on the menu now).