Bug Tracker

Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#8926 closed bug (invalid)

Bay Area jQuery Conference T-Shirts do not follow the style guide.

Reported by: ralphholzmann Owned by: john
Priority: blocker Milestone: 1.6
Component: unfiled Version: 1.6b1
Keywords: Cc:
Blocked by: Blocking:

Description

Hello,

I attended the Bay Area jQuery Conference last weekend and was delighted to receive a stylish and high quality jQuery t-shirt. The front of the t-shirt says $("body").wrap(this);, while the back has the jQuery logo close to the top.

Upon further reflection, it dawned on me that the t-shirt does not follow the jQuery Core Style Guidelines. Per http://docs.jquery.com/JQuery_Core_Style_Guidelines#Function_Calls, it states "Always include extra spaces around the arguments." Considering that both the $ and wrap are function calls, the t-shirt should read $( "body" ).wrap( this );

This bug is a blocker for all future jQuery Conferences and I expect a patch to go out within the next couple of days. A sewable patch is preferred.

Thank you,

Ralph Holzmann

Change History (13)

comment:1 Changed 9 years ago by miketaylr

I can't reproduce, but I wasn't at the conference.

comment:2 Changed 9 years ago by timmywil

Component: unfiledbuild
Milestone: 1.next1.6
Owner: set to john
Priority: undecidedblocker
Status: newassigned
Version: git1.6b1

Actually, spaces are not required when double quotes are present (since they provide a visible space by themselves), so it would be

$("body").wrap( this );

comment:3 Changed 9 years ago by Rick Waldron

Confirmed.

Additionally, throws an uncaught type error exception, see the test case:

http://jsfiddle.net/rwaldron/RSdAg/

comment:4 Changed 9 years ago by dmethvin

In the meantime, here is a workaround: Wear a size "Small" t-shirt and it will stretch to add some more space between the characters.

comment:5 in reply to:  4 Changed 9 years ago by Nikola

Replying to dmethvin:

In the meantime, here is a workaround: Wear a size "Small" t-shirt and it will stretch to add some more space between the characters.

Confirmed, this works. It may not be pretty but it is functional.

comment:6 Changed 9 years ago by john

Resolution: patchwelcome
Status: assignedclosed

Unfortunately the release is already out the door so it's a bit late. Let's bring this up during the next t-shirt roadmap discussion. In the meantime, patches welcome.

comment:7 Changed 9 years ago by anonymous

saw this from twitter. Brilliant

comment:8 Changed 9 years ago by dougneiner

The code did follow the style guide, but it was minified for production. We also recommend storing the shirts in a gzip-lock bag to keep the size small.

Additionally, after evaluating where this code would normally run, we realized we didn't need it to run in a closure. Unfortunately this means it is not compatible with jQuery running in .noConflict mode as $ is expected to equal jQuery. We do not recommend layering this shirt on top of a MooTools or Dojo shirt if possible. If you want to combine them, you will probably want to put the jQuery conference shirt on first.

comment:9 Changed 9 years ago by cowboy

Resolution: patchwelcome
Status: closedreopened

I'm going to release this revised t-shirt first as a plugin, and if you guys want me to add it into core, I have no problem with that. Just bring it up at the next roadmap meeting.

http://benalman.com/grab/5c2e39.png

comment:10 Changed 9 years ago by Robert Katić

No need to wait DOM ready - it is obviously a one page application, where tshirt was dynamically injected.

comment:11 Changed 9 years ago by jswartwood@…

@rkatic, likely that guy in @cowboy's plugin snapshot is named Dominic... I would also recommend waiting for Dom ready before trying wrap. Otherwise he would not allow it I.e. "operation aborted".

comment:12 Changed 9 years ago by john

Component: buildunfiled

comment:13 Changed 9 years ago by john

Resolution: invalid
Status: reopenedclosed

Ok, we're closing bugs for 1.6, to make sure we have no blockers. We can have more fun later ;)

Note: See TracTickets for help on using tickets.