-
Notifications
You must be signed in to change notification settings - Fork 484
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
Merging attributes page into Manipulating Elements #713
base: main
Are you sure you want to change the base?
Merging attributes page into Manipulating Elements #713
Conversation
…om the attributes page
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
@connorcartwright Thank for the PR :-) Looks good to me. @AurelioDeRosa Does this look good to you? |
@@ -132,23 +132,33 @@ myList.append( myItems.join( "" ) ); | |||
|
|||
## Manipulating Attributes | |||
|
|||
jQuery's attribute manipulation capabilities are extensive. Basic changes are simple, but the `.attr()` method also allows for more complex manipulations. It can either set an explicit value, or set a value using the return value of a function. When the function syntax is used, the function receives two arguments: the zero-based index of the element whose attribute is being changed, and the current value of the attribute being changed. | |||
jQuery's attribute manipulation capabilities are extensive. | |||
Basic changes are simple, but the `.attr()` method also allows for more complex manipulations. |
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.
Probably this line and the next can be combined with the previous one. I'm not sure we need break lines in between.
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 agree with @AurelioDeRosa on this one
I've added a few comments to improve the PR. Overall I'm excited to see that we have a new contributor. Thank you @connorcartwright! |
Hey @AurelioDeRosa No problem, thank you for the feedback! I've removed the line spacing between those two sentences and added a link to the Would it be best to add the
Or is there a simpler way I'm missing? |
I think we might have a process in place for these situations but I'd like to have our expert opinion @gnarf |
Hey guys @AurelioDeRosa @agcolom
Following the comments made in issue #642 by devs4u I’ve merged the attributes page into the manipulating elements page.
I've added some line spacing and moved around the text a bit in the Manipulating Attributes section. I also added in a sentence from the old attributes page and a getter code example.
Let me know if you have an feedback or if there are any problems and I'll get on it asap