Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Clarify where ngDisabled should be used #15473

Closed
wants to merge 1 commit into from

Conversation

samuraijane
Copy link

@samuraijane samuraijane commented Dec 7, 2016

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.

My understanding is that ngDisabled is for form elements only so I propose that the documentation reflect this more clearly.
@googlebot
Copy link

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. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

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. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@@ -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
Copy link
Member

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...

Copy link
Contributor

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?

Copy link
Member

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).

Copy link
Contributor

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.)

@gkalpak gkalpak added this to the Backlog milestone Dec 7, 2016
@gkalpak gkalpak modified the milestones: Purgatory, Backlog Dec 7, 2016
@samuraijane
Copy link
Author

():

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@Narretz
Copy link
Contributor

Narretz commented Dec 7, 2016

@samuraijane No need to close it, you can update the PR.

@samuraijane
Copy link
Author

@Narretz Thank you. Still learning how to do this.

@samuraijane samuraijane reopened this Dec 7, 2016
@Narretz
Copy link
Contributor

Narretz commented Dec 15, 2016

@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.

@Narretz Narretz closed this in 3c259ce Jan 25, 2017
Narretz added a commit that referenced this pull request Jan 27, 2017
ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants