Enable sidebar register function to get the sidebars in sidebar container across all the app #77
Conversation
| @@ -76,8 +75,7 @@ import { isBrowser, upperCaseFirst, isLTR, isIOS } from './utils'; | |||
| transition: transform 0.3s cubic-bezier(0, 0, 0.3, 1); | |||
| } | |||
| `], | |||
| changeDetection: ChangeDetectionStrategy.OnPush, | |||
| encapsulation: ViewEncapsulation.None | |||
There was a problem hiding this comment.
I only removed ViewEncapsulation.None. In my opinion, I prefer to use the default value if it is possible (emulated) as a solution to the future of browsers with shadow dom. Do you think that is better ViewEncapulation.None or Emulated?
There was a problem hiding this comment.
I suppose it's fine to keep it as the default. Not entirely sure if that'll affect users though.
There was a problem hiding this comment.
The users needs to use the deep (>>>) on css to modify the style of the sidebar if the css file is loaded by angular. On other cases, the deep selector is not necessary.
src/sidebar.service.ts
Outdated
| @@ -15,11 +19,19 @@ export class SidebarService { | |||
| this._closeObserver.next(); | |||
| } | |||
|
|
|||
| registry(sidebar: Sidebar) { | |||
There was a problem hiding this comment.
I think that you have reason. Register is better word for the function.
src/sidebar-container.component.ts
Outdated
| @@ -260,7 +248,7 @@ export class SidebarContainer implements AfterContentInit, OnChanges, OnDestroy | |||
| if (this._sidebars && this.allowSidebarBackdropControl) { | |||
| let hasOpen = false; | |||
|
|
|||
| const _sidebars = this._sidebars.toArray(); | |||
| const _sidebars: Sidebar[] = [...this._sidebars]; | |||
There was a problem hiding this comment.
Possibly not. We can use directly
const _sidebars = this._sidebars;
or use the property of the class directly as for argument.
There was a problem hiding this comment.
Yeah I think we can just directly use it if it's already an array.
src/sidebar-container.component.ts
Outdated
| @@ -87,22 +82,21 @@ export class SidebarContainer implements AfterContentInit, OnChanges, OnDestroy | |||
| @Output() showBackdropChange = new EventEmitter<boolean>(); | |||
|
|
|||
| /** @internal */ | |||
| @ContentChildren(Sidebar) | |||
| _sidebars: QueryList<Sidebar>; | |||
| _sidebars: Sidebar[] = new Array<Sidebar>(); | |||
There was a problem hiding this comment.
I prefer just [] over the new Array() style.
Sifawm
changed the title
|
@arkon do you see any changes more? My team is using this changes on a real project without any problem. |
|
@Sifawm Yup, I'm on vacation right now though so I don't have access to a laptop to really do much, so I'll merge/publish when I get back home at the end of the week. |
|
@arkon my apologises. Enjoy your vacations. |
New function inside SidebarService to registry sidebar with observables.