-
Notifications
You must be signed in to change notification settings - Fork 27.4k
Clarify where ngDisabled should be used #15473
Conversation
My understanding is that ngDisabled is for form elements only so I propose that the documentation reflect this more clearly.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
@@ -159,7 +159,7 @@ | |||
* | |||
* @description | |||
* | |||
* This directive sets the `disabled` attribute on the element if the | |||
* This directive sets the `disabled` attribute on a form element if the |
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 think this can be more confusing than helpful. ("form element" often refers to the <form>
element.)
How about:
This directive sets the
disabled
attribute on the element (typically a form control) if the...
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 think there are only these element that natively respond to disabled: option, input, textarea, button, select. Why not list them all?
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.
There are more (e.g. optgroup, fieldset). Plus there is nothing stopping people from using it on elements that don't natively respond to disabed
, such as custom controls. I wouldn't make too strong a statement (i.e. "it can be used on these elements only). Mentioning the common usecase is fine though (either under the general term "form controls" or by explicit listing).
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.
Yeah, I meant list all the elements that are supported natively, but not write that they can be used only on these elements. I'm fine with
(typically a form control, e.g.
input
,button
,select
etc.)
(): |
CLAs look good, thanks! |
1 similar comment
CLAs look good, thanks! |
@samuraijane No need to close it, you can update the PR. |
@Narretz Thank you. Still learning how to do this. |
@samuraijane are you going to update this PR? If not, I'm gonna close it, and you can reopen it once you have time to update it. |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Update documentation.
What is the current behavior? (You can also link to an open issue here)
n/a
What is the new behavior (if this is a feature change)?
n/a
Does this PR introduce a breaking change?
No
Please check if the PR fulfills these requirements
Other information:
My understanding is that ngDisabled is for form elements only so I propose that the documentation reflect this more clearly.