events/basics: fix example #587
Conversation
|
Can you update this so that we create the element with the text and class already included? In this way we aren't muddying up the method chain with extra methods that we can do right in the jQuery function. It feels easier to read and try to understand when looking at the line then with three methods immediately after the jQuery function. |
|
FWIW, my preference is to go through the jQuery methods because as a newcomer, seeing all the HTML as a string tends to encourage the very unsafe habit of concatenating strings together without escaping user data. |
|
@RedWolves Of course, PR updated. Exactly because they're newcomers we should show them the right way, IMHO. |
|
@scottgonzalez do we have any style guidelines around this or is just preference? I looked around to see if I could find a pattern but didn't see anything concrete. |
|
We don't have any guidelines around this. |
| @@ -42,7 +42,7 @@ $( document ).ready(function(){ | |||
| // Now create a new button element with the alert class. This button | |||
| // was created after the click listeners were applied above, so it | |||
| // will not have the same click behavior as its peers | |||
| $( "button" ).addClass( "alert" ).appendTo( document.body ); | |||
| $( "<button class='alert'>Alert!</button>" ).addClass( "alert" ).appendTo( document.body ); | |||
There was a problem hiding this comment.
You're now inlining the class and adding after the fact.
Fix for #584. This fixes the broken example in
events/basics.Two things I changed:
$( "button" )to$( "<button>" ), which was our original intent. This creates a new button to be appended to the body, instead of selecting the existing one. That was the original idea of the example (see comments L42-44).text()to that newly created button, technically it works without but it doesn't feel right and it's confusing.