Update combobox and calendar with directives #95
Conversation
Add directives for the ComboBox and Calendar to help support Angular template i18n patterns. Change wrapper to support new directives, and fix a subtle binding bug. Update fabric component to demo new functionality.
bengry
left a comment
bengry
left a comment
There was a problem hiding this comment.
Looks good overall.
There are some minor things to change, and some I'd like to understand better on why they were made (especially in wrapper-component.ts
apps/docs/src/app/containers/component-docs/fabric/fabric.component.html
Show resolved
Hide resolved
libs/fabric/src/lib/components/calendar/directives/calendar-strings-directive.component.ts
Show resolved
Hide resolved
libs/fabric/src/lib/components/combo-box/directives/combo-box-option.directive.ts
Outdated
Show resolved
Hide resolved
-Used Prettify to format some files -Changed 'components' to 'declrations' in certain module files -Adjusted ngAfterContentInit in children of wrapper-component to call super.ngAfterContentInit to preserve lifecycle event ordering. This is needed because of moving a lifecycle event from ngAfterViewInit to ngAfterContentInit in the wrapper component.
apps/docs/src/app/containers/component-docs/fabric/fabric.component.html
Outdated
Show resolved
Hide resolved
| */ | ||
| @Directive({ selector: 'fab-combo-box-option' }) | ||
| export class ComboBoxOptionDirective { | ||
| @Input() optionKey: IComboBoxOption['key']; |
There was a problem hiding this comment.
Why not call it key in the property name too? We'd like to stay consistant with the office-ui-fabric-react names as much as possible, especially with stuff like #91 in the pipeline.
There was a problem hiding this comment.
I concur, but that runs foul of the wrapper-component _isForbiddenAttribute method. This causes the ComboBox to misbehave (it ignores the key attribute binding and triggers a console warning).
There was a problem hiding this comment.
Ok, makes sense, let's keep it like that then. Please do add some JDoc on this to make a note of this.
libs/fabric/src/lib/components/combo-box/directives/combo-box-option.directive.ts
Outdated
Show resolved
Hide resolved
bengry
left a comment
bengry
left a comment
There was a problem hiding this comment.
Looks good, just the JDoc thing is missing. Please add it and then I'll make the merge to master. I'd do it myself, but I don't have push permissions on your fork (probably).
2 participants
Add directives for the ComboBox and Calendar to enable the Angular-style template localization pattern.
Change wrapper to fix some timing bugs