Skip to content
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

events/basics: fix example #587

Closed
wants to merge 1 commit into from
Closed

Conversation

arthurvr
Copy link
Member

Fix for #584. This fixes the broken example in events/basics.

Two things I changed:

  • 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)
  • Also added some .text() to that newly created button, technically it works without but it doesn't feel right and it's confusing.

@RedWolves
Copy link
Member

Can you update this so that we create the element with the text and class already included?

$( "<button class='alert'>Alert!</button>" ).appendTo( document.body );

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.

@scottgonzalez
Copy link
Member

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.

@arthurvr
Copy link
Member Author

@RedWolves Of course, PR updated. Exactly because they're newcomers we should show them the right way, IMHO.

@RedWolves
Copy link
Member

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

@scottgonzalez
Copy link
Member

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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're now inlining the class and adding after the fact.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants