-
Notifications
You must be signed in to change notification settings - Fork 6.8k
chore: hide internal properties from selection-list api doc #8841
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
chore: hide internal properties from selection-list api doc #8841
Conversation
crisbeto
left a comment
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, but deferring to @jelbourn regarding the constructor signature changes.
src/lib/list/selection-list.ts
Outdated
| private _changeDetector: ChangeDetectorRef, | ||
| @Optional() @Inject(forwardRef(() => MatSelectionList)) | ||
| public selectionList: MatSelectionList) { | ||
| private _selectionList: MatSelectionList) { |
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.
I doubt that many people are depending on this property in particular, but this is technically a breaking change.
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.
Good point. If really necessary we can deprecate it
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.
Might as well leave it, doesn't really hurt to leave it as public.
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.
Hm, but it doesn't really make sense from an API perspective. Also it shows up in the docs which is kind of unnecessary. I don't really mind keeping that property as it is. Just let me know.
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.
Can just make it docs-private
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.
Done.
5248d55 to
e97c69c
Compare
e97c69c to
f04b1f9
Compare
crisbeto
left a comment
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
src/lib/list/selection-list.ts
Outdated
| @Input() | ||
| get disabled() { return (this.selectionList && this.selectionList.disabled) || this._disabled; } | ||
| get disabled() { | ||
| return this._disabled || this.selectionList && this.selectionList.disabled; |
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.
Add the parentheses around the second condition here? Otherwise it's a little ambiguous.
* Recently with the NgModel support for the selection list, the `onTouched` property has been added. This one needs to be prefixed with an underscore to be hidden in the docs. * Since the initial implementation of the selection-list the `selectionList` variable has been initialized publically in the constructor of the `MatListOption` and therefore shows up in the docs. This property has been made private now.
f04b1f9 to
9cd5a73
Compare
jelbourn
left a comment
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
…8841) * Recently with the NgModel support for the selection list, the `onTouched` property has been added. This one needs to be prefixed with an underscore to be hidden in the docs. * Since the initial implementation of the selection-list the `selectionList` variable has been initialized publically in the constructor of the `MatListOption` and therefore shows up in the docs. This property has been made private now.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
onTouchedproperty has been added. This one needs to be prefixed with an underscore to be hidden in the docs.selectionListvariable has been initialized publically in the constructor of theMatListOptionand therefore shows up in the docs. This property has been made private now.