Review Request: JavaScript libraries updates, add small JS utilities (synopsis length, automatic keydates)
54 views
Skip to first unread message
Alexandros Diamantidis
Aug 29, 2012, 8:01:51 PM8/29/12
to GGD Tech Group, Alexandros Diamantidis
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.comics.org/r/1027/
-----------------------------------------------------------
Review request for GGD Tech Group.
Summary
-------
This change began as an addition of two small UI improvements using jQuery: A running length indication while entering the synopsis and a function to automaticaly detect the keydate from the pubdate field (per bug #684).
Because the easiest way to process input while typing is the "input" JS event which isn't supported in IE < 9, I imported the HTML5fixes jQuery plugin which simulates it when needed. I also took the liberty of updating our other JS libraries to the latest versions (jQuery v1.7.2, Jcrop v0.9.10) - not actually needed, but it doesn't hurt.
This addresses bug 684.
http://dev.comics.org/bugs/show_bug.cgi?id=684
Diffs
-----
/pydjango/media/jquery/js/jquery.html5fixes.js PRE-CREATION
/pydjango/media/jquery/js/jquery.min.js 1487
/pydjango/apps/oi/forms.py 1487
/pydjango/apps/oi/views.py 1487
/pydjango/media/jquery/css/jquery.Jcrop.css 1487
/pydjango/media/jquery/js/jquery.Jcrop.min.js 1487
/pydjango/media/js/oi/revision_form_utils.js PRE-CREATION
/pydjango/templates/gcd/base.html 1487
/pydjango/templates/oi/bits/jquery.html PRE-CREATION
/pydjango/templates/oi/bits/revision_form_utils.html PRE-CREATION
/pydjango/templates/oi/bits/select_cover_section.html 1487
/pydjango/templates/oi/edit/add_frame.html 1487
/pydjango/templates/oi/edit/revision.html 1487
/pydjango/templates/oi/edit/upload_gatefold_cover.html 1487
Diff: http://reviews.comics.org/r/1027/diff
Testing
-------
Tested the new functions and the gatefold cover selection (Jcrop) on Firefox, Opera, Chromium and IE 6 and 10-preview. The only problem on IE6 is that if you paste something in the synopsis using only the mouse (not Ctrl-V for example) the event doesn't fire and the synopsis length isn't updated. Just clicking on the textarea or pressing a key updates it.
Thanks,
Alexandros
Jochen G.
Sep 6, 2012, 7:24:44 PM9/6/12
to gcd-...@googlegroups.com
too busy till next week to look at the code (it might be simple, but I
just won't look at code reviews).
Jochen
just won't look at code reviews).
Jochen
Alexandros Diamantidis
Sep 7, 2012, 6:30:33 AM9/7/12
to gcd-...@googlegroups.com
Don't worry, take your time. I assumed that you were busy.
By the way, here's some more info on the change for when look at the
code:
The interesting JS code is in media/js/oi/revision_form_utils.js.
I factored out the jQuery loading in templates/oi/bits/jquery.html
I added a new block 'footer' in templates/gcd/base.html so that any
JS loading can go in the end of the page - this seems to be better,
since it avoids blocking the rendering of the page if for some reason
the JS is slow to load, as I read in many articles on how to deploy
jQuery.
I'm passing the synopsis maximum length as a variable - it was hardcoded
in a couple of places. This necessitated adding the settings to the
request context in some cases.
The reason I updated all JS libraries at the same time is that since I
was going to test various browsers for compatibility problems anyway, it
made sense to do those updates at the same time, to consolidate testing.
Thanks!
Alexandros
* Jochen G. [2012-09-07 02:24]:
By the way, here's some more info on the change for when look at the
code:
The interesting JS code is in media/js/oi/revision_form_utils.js.
I factored out the jQuery loading in templates/oi/bits/jquery.html
I added a new block 'footer' in templates/gcd/base.html so that any
JS loading can go in the end of the page - this seems to be better,
since it avoids blocking the rendering of the page if for some reason
the JS is slow to load, as I read in many articles on how to deploy
jQuery.
I'm passing the synopsis maximum length as a variable - it was hardcoded
in a couple of places. This necessitated adding the settings to the
request context in some cases.
The reason I updated all JS libraries at the same time is that since I
was going to test various browsers for compatibility problems anyway, it
made sense to do those updates at the same time, to consolidate testing.
Thanks!
Alexandros
* Jochen G. [2012-09-07 02:24]:
Jochen G.
Sep 10, 2012, 2:48:23 PM9/10/12
to GGD Tech Group, Jochen G., Alexandros Diamantidis
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
-----------------------------------------------------------
I don't know if we really want to support IE < 9 by special fixes. It is less than 15% and dropping.
/pydjango/apps/oi/forms.py
<http://reviews.comics.org/r/1027/#comment1431>
where did this go ?
/pydjango/templates/oi/edit/add_frame.html
<http://reviews.comics.org/r/1027/#comment1432>
if we call it footer, it should be after the body, or ?
/pydjango/templates/oi/edit/add_frame.html
<http://reviews.comics.org/r/1027/#comment1433>
again, where did this go ?
/pydjango/templates/oi/edit/upload_gatefold_cover.html
<http://reviews.comics.org/r/1027/#comment1430>
footer / body - order
- Jochen
On 2012-08-30 00:01:51, Alexandros Diamantidis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.comics.org/r/1027/
> -----------------------------------------------------------
>
Alexandros Diamantidis
Sep 11, 2012, 5:24:40 AM9/11/12
to GGD Tech Group, Jochen G., Alexandros Diamantidis
> On 2012-09-10 18:48:24, Jochen G. wrote:
> > I don't know if we really want to support IE < 9 by special fixes. It is less than 15% and dropping.
Of course if we encounter any real difficulties, we can always change our minds ;)
> On 2012-09-10 18:48:24, Jochen G. wrote:
> > /pydjango/apps/oi/forms.py, line 828
> > <http://reviews.comics.org/r/1027/diff/1/?file=7576#file7576line828>
> >
> > where did this go ?
See line 94 in revision_form_utils.js:
> > where did this go ?
// Disable form submission with Enter for barcode field
$('#id_barcode').keypress(function (e) {
if (e.which == 13) {
return false;
}
});
This way, we don't need to put any JS code in our forms.
If we want to do this for other fields too, we can just add their id's (or classes) in the $('#id_barcode') selector. And if the situation becomes more complex, we can even write a small jQuery plugin for this.
> On 2012-09-10 18:48:24, Jochen G. wrote:
> > /pydjango/templates/oi/edit/add_frame.html, lines 23-33
> > <http://reviews.comics.org/r/1027/diff/1/?file=7587#file7587line23>
> >
> > again, where did this go ?
In revision_form_utils.js, see above ;)
> > again, where did this go ?
> On 2012-09-10 18:48:24, Jochen G. wrote:
> > /pydjango/templates/oi/edit/add_frame.html, lines 16-20
> > <http://reviews.comics.org/r/1027/diff/1/?file=7587#file7587line16>
> >
> > if we call it footer, it should be after the body, or ?
Well, maybe "footer" isn't the best name - mainly it's the place where we'll put the scripts if there's no reason to put them elsewhere. I just saw some people calling this place the "footer" in the articles I read, but we could also call it "bottom", "page_end" or something similar.
> > if we call it footer, it should be after the body, or ?
Now, in our templates I think the proper place to load the JavaScript is right next to the CSS, even if (now that I've added the "footer" block) they'll appear in a different part in the final HTML output. I think this grouping makes it a bit cleaner, and the placement doesn't really matter to Django.
- Alexandros
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.comics.org/r/1027/#review2101
-----------------------------------------------------------
Jochen G.
Sep 11, 2012, 4:18:03 PM9/11/12
to GGD Tech Group, Jochen G., Alexandros Diamantidis
> On 2012-09-10 18:48:24, Jochen G. wrote:
> > I don't know if we really want to support IE < 9 by special fixes. It is less than 15% and dropping.
>
>
> Alexandros Diamantidis wrote:
> Well, it doesn't cost us much. The main incompatibility here concerns the events that fire when someone types in a textarea or input field. With newer browsers you can just bind the "input" event, while in IE < 9 and I think somewhere else you need to bind other events as well. The html5fixes plugin canonicalizes this by simulating the "input" event where it doesn't exist, so there's no need for special fixes in our code.
>
> Of course if we encounter any real difficulties, we can always change our minds ;)
It costs us an additional file to maintain/have with little gain. These changes are nice, but uncritical, so we don't loose much by not having these for the 15% (and dropping) affected.
> Well, it doesn't cost us much. The main incompatibility here concerns the events that fire when someone types in a textarea or input field. With newer browsers you can just bind the "input" event, while in IE < 9 and I think somewhere else you need to bind other events as well. The html5fixes plugin canonicalizes this by simulating the "input" event where it doesn't exist, so there's no need for special fixes in our code.
>
> Of course if we encounter any real difficulties, we can always change our minds ;)
> On 2012-09-10 18:48:24, Jochen G. wrote:
> > /pydjango/templates/oi/edit/add_frame.html, lines 23-33
> > <http://reviews.comics.org/r/1027/diff/1/?file=7587#file7587line23>
> >
> > again, where did this go ?
>
>
> Alexandros Diamantidis wrote:
> In revision_form_utils.js, see above ;)
The separate handling for IE is not necessary anymore ? The onKeyPress is not necessary anymore ?
> In revision_form_utils.js, see above ;)
> On 2012-09-10 18:48:24, Jochen G. wrote:
> > /pydjango/templates/oi/edit/add_frame.html, lines 16-20
> > <http://reviews.comics.org/r/1027/diff/1/?file=7587#file7587line16>
> >
> > if we call it footer, it should be after the body, or ?
>
>
> Alexandros Diamantidis wrote:
> Well, maybe "footer" isn't the best name - mainly it's the place where we'll put the scripts if there's no reason to put them elsewhere. I just saw some people calling this place the "footer" in the articles I read, but we could also call it "bottom", "page_end" or something similar.
>
> Now, in our templates I think the proper place to load the JavaScript is right next to the CSS, even if (now that I've added the "footer" block) they'll appear in a different part in the final HTML output. I think this grouping makes it a bit cleaner, and the placement doesn't really matter to Django.
It is recommended to load javascript as the last part of a webpage, or ? So it can be called footer, as it should be the last part of the page.
> Well, maybe "footer" isn't the best name - mainly it's the place where we'll put the scripts if there's no reason to put them elsewhere. I just saw some people calling this place the "footer" in the articles I read, but we could also call it "bottom", "page_end" or something similar.
>
> Now, in our templates I think the proper place to load the JavaScript is right next to the CSS, even if (now that I've added the "footer" block) they'll appear in a different part in the final HTML output. I think this grouping makes it a bit cleaner, and the placement doesn't really matter to Django.
JS doesn't really have much to do with CSS. The order of things in our templates should reflect the order in which is loaded and the order in the base.html, otherwise it can get confusing.
- Jochen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.comics.org/r/1027/#review2101
-----------------------------------------------------------
Jochen G.
Oct 2, 2012, 4:59:06 PM10/2/12
to GGD Tech Group, Jochen G., Alexandros Diamantidis
| This is an automatically generated e-mail. To reply, visit: http://reviews.comics.org/r/1027/ |
I still don't really like the additional file for IE < 9, we don't necessarily need to add this functionality for the older browsers which disappear anyway in time. So I would prefer to remove it. If you can answer my other question ? Otherwise I would like to have the footer actually as the last part of the page, after body. If I am not mistaken this is where javascript should be put, so that it doesn't hinder the loading of the rest.
| /pydjango/media/js/oi/revision_form_utils.js (Diff revision 1) | |||
|---|---|---|---|
None |
{'text': 'function parsePubDate (pubDate) {', 'line': 44} |
||
| 94 | |||
// Disable form submission with Enter for barcode field |
| 95 | $('#id_barcode').keypress(function (e) { |
||
|---|---|---|---|
| 96 | if (e.which == 13) { |
||
| 97 | return false; |
||
| 98 | } |
||
| 99 | }); |
Is the separate handling for IE is not necessary anymore ?
- Jochen
On August 30th, 2012, 12:01 a.m., Alexandros Diamantidis wrote:
|
Review request for GGD Tech Group.
|
By Alexandros Diamantidis.
Updated Aug. 30, 2012, 12:01 a.m. Description |
|
Testing |
|
Bugs:
684
Diffs
|
Andres Jimenez
Oct 3, 2012, 1:26:23 AM10/3/12
to gcd-...@googlegroups.com, Jochen G., Alexandros Diamantidis
It looks like the migrated reviewboard works :-)
--
Andres Jimenez
--
GCD-Tech mailing list - gcd-...@googlegroups.com
To unsubscribe send email to gcd-tech+u...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/gcd-tech
Andres Jimenez
Alexandros Diamantidis
Oct 3, 2012, 11:17:29 AM10/3/12
to Jochen G., GCD Tech Group, Alexandros Diamantidis
| This is an automatically generated e-mail. To reply, visit: http://reviews.comics.org/r/1027/ |
On October 2nd, 2012, 8:59 p.m., Jochen G. wrote:
I still don't really like the additional file for IE < 9, we don't necessarily need to add this functionality for the older browsers which disappear anyway in time. So I would prefer to remove it. If you can answer my other question ? Otherwise I would like to have the footer actually as the last part of the page, after body. If I am not mistaken this is where javascript should be put, so that it doesn't hinder the loading of the rest.
Sorry for dragging my feet on this... I intend rework it a bit and submit an updated request. I'll move the JS loading as you indicated and use a different jQuery plugin for the input events - see: http://whattheheadsaid.com/projects/input-special-event - if you read the comments there, you'll see that even IE9 has some problems with input events and this tries to work around them.
On October 2nd, 2012, 8:59 p.m., Jochen G. wrote:
/pydjango/media/js/oi/revision_form_utils.js (Diff revision 1) {'text': 'function parsePubDate (pubDate) {', 'line': 44}94 // Disable form submission with Enter for barcode field95 96 97 98 }99 });Is the separate handling for IE is not necessary anymore ?
Yes, the nice thing about jQuery's keypress events is that they normalize the property that holds they key - so no separate handling needed.
- Alexandros
On August 30th, 2012, 12:01 a.m., Alexandros Diamantidis wrote:
|
Review request for GCD Tech Group. |
Alexandros Diamantidis
Oct 5, 2012, 6:17:45 AM10/5/12
to Jochen G., GCD Tech Group, Alexandros Diamantidis
| This is an automatically generated e-mail. To reply, visit: http://reviews.comics.org/r/1027/ |
|
Review request for GCD Tech Group.
By Alexandros Diamantidis.
|
Updated Oct. 5, 2012, 12:17 p.m. Changes
Description |
|
Testing |
|
Bugs:
684
Diffs (updated)
|
Jochen G.
Oct 5, 2012, 5:57:14 PM10/5/12
to GCD Tech Group, Jochen G., Alexandros Diamantidis
| This is an automatically generated e-mail. To reply, visit: http://reviews.comics.org/r/1027/ |
Hmm, you forgot the new files, or ? And btw, the updates of the existing jquery thingies don't need to be in the review, it's not like we are reviewing them. The one new external one is of some interest, since we add it, but a link to a webpage or so would also be fine.
- Jochen
On October 5th, 2012, 12:17 p.m., Alexandros Diamantidis wrote:
|
Review request for GCD Tech Group.
By Alexandros Diamantidis.
Updated Oct. 5, 2012, 12:17 p.m. |
Description |
Testing
Bugs:
684
Diffs |
Alexandros Diamantidis
Oct 6, 2012, 8:37:29 PM10/6/12
to Jochen G., GCD Tech Group, Alexandros Diamantidis
| This is an automatically generated e-mail. To reply, visit: http://reviews.comics.org/r/1027/ |
On October 5th, 2012, 11:57 p.m., Jochen G. wrote:
Hmm, you forgot the new files, or ? And btw, the updates of the existing jquery thingies don't need to be in the review, it's not like we are reviewing them. The one new external one is of some interest, since we add it, but a link to a webpage or so would also be fine.
I did, sorry! Got confused moving the chanhes to git from SVN. I thought I'd include all JS libraries as a way to allow anyone testing this change to install them easily (by just applying the diff from this review) but you're right that it makes the review request much more confusing. So, I did as asked and just mention them in the description.
- Alexandros
Alexandros Diamantidis
Oct 6, 2012, 8:37:42 PM10/6/12
to Jochen G., GCD Tech Group, Alexandros Diamantidis
| This is an automatically generated e-mail. To reply, visit: http://reviews.comics.org/r/1027/ |
|
Review request for GCD Tech Group.
By Alexandros Diamantidis.
|
Updated Oct. 7, 2012, 2:37 a.m. Changes
|
Description
Testing
Bugs:
684
Diffs (updated) |
|
|
Jochen G.
Oct 7, 2012, 6:16:55 PM10/7/12
to GCD Tech Group, Jochen G., Alexandros Diamantidis
| This is an automatically generated e-mail. To reply, visit: http://reviews.comics.org/r/1027/ |
Ship it!
This looks good now. Please submit to the git, but don't deploy, the change from svn to git still needs to be done on production, but we have local changes due to adds, which won't be committed for now... Thanks.
- Jochen
On October 7th, 2012, 2:37 a.m., Alexandros Diamantidis wrote:
|
Review request for GCD Tech Group.
By Alexandros Diamantidis.
Updated Oct. 7, 2012, 2:37 a.m. |
Description |
Testing
Bugs:
684
Diffs |
Reply all
Reply to author
Forward
0 new messages