Closed Bug 563236 Opened 15 years ago Closed 14 years ago

[homepage] externalize all media resource and bundle what can be bundled

Categories

(Webtools Graveyard :: Elmo, defect, P2)

Product:
Webtools Graveyard
Component:
Elmo
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: Pike, Assigned: peterbe)

References

Details

Attachments

(1 file, 6 obsolete files)

Corrections to all l10nstats inline css -> external
669.72 KB, patch
Pike
: review+
Details | Diff | Splinter Review
We should have a command similar to pinax' build_media to gather static media from all installed apps and put them into a single dir, served as static content. I've been pondering to use MEDIA_ROOT and MEDIA_URL, but I'm not sure if the two are a bug or a feature. MEDIA_URL would need to get adjusted for each installation, in particular for those not serving from '/'. Which might be a feature, as for example https://l10n-stage-sj.mozilla.org/test/ would share the static media with https://l10n-stage-sj.mozilla.org/ by default. I'm not sure if we should add another /site-media static url, or try to reuse the /static url in some fashion. Comments? This is somewhat urgent, as it'd help adding the todo app to l10n_site.
At a glance, could we just use http://bitbucket.org/jezdez/django-staticfiles ?
Looks good. Have you had a chance to test it yet?
-> stas
Assignee: nobody → stas
The new URL is https://github.com/jezdez/django-staticfiles. The docs are at http://pinaxproject.com/docs/dev/media.html. I'll test it and report back.
Probably gonna get that out of playdoh.
Assignee: stas → nobody
Component: Infrastructure → Elmo
Depends on: 652793
Product: Mozilla Localizations → Webtools
QA Contact: infrastructure → elmo
Summary: [dashboard][homepage] add build_media command → [homepage] add build_media command
Version: unspecified → 1.0
Should get fixed as part of the playdoh move.
Assignee: nobody → peterbe
Target Milestone: --- → 1.2
To be clear: we will need to make code changes, but we should be able to fix this as part of the playdoh move, since playdoh has support for this.
Feature branch uploaded here: https://github.com/peterbe/elmo/tree/feature/externalize-all-media @Pike, would you mind pulling it locally and test that it works?
Blocks: 658966
Blocks: 653205
Summary: [homepage] add build_media command → [homepage] externalize all media resource and bundle what can be bundled
I like the concept here. I found some inconsistencies, at least on the two graph views, in the locale progress the legend is moving from one wrong position to another worse position, and in the tree progress, the histogram bars get a black border, which they shouldn't. At least the latter is due to conflicting CSS rules that are now in different order. For the latter, the fix is to remove the CSS rule that makes the border black, that's left-overs from even more crappy styling. I did like the redo of the graphs js, though I'd rather not have empty lines in the data array. Also, make the generated js pretty if possible, I saw code like var foo = 3 , bar = 5 , stuff = "stuff" ; That reads like buggy code while it isn't. I don't like that the static stuff is now in media, and not in their appdirs, can we use the staticfiles app for that? Maybe build the media outside of the source dir for deployment?
(In reply to comment #10) > I like the concept here. > > I found some inconsistencies, at least on the two graph views, in the locale > progress the legend is moving from one wrong position to another worse > position, and in the tree progress, the histogram bars get a black border, > which they shouldn't. At least the latter is due to conflicting CSS rules > that are now in different order. For the latter, the fix is to remove the > CSS rule that makes the border black, that's left-overs from even more > crappy styling. > Where? What? I'd love to fix that but I don't know what you refer to. > I did like the redo of the graphs js, though I'd rather not have empty lines > in the data array. Also, make the generated js pretty if possible, I saw > code like > > var foo = 3 > , bar = 5 > , stuff = "stuff" > ; > > That reads like buggy code while it isn't. > That is correct style. Google Closure linter accepts it just fine. This style of syntax is very common in Javascript (especially in NodeJS) as it makes it easy to comment out elements without accidentally make the statement end with comma before the semi-colon. > I don't like that the static stuff is now in media, and not in their > appdirs, can we use the staticfiles app for that? Maybe build the media > outside of the source dir for deployment? Yeah, I'd like to get to the bottom of that too. First I'm just eager to get this basic refactoring in place.
the border in https://github.com/peterbe/elmo/blob/feature%2Fexternalize-all-media/apps/l10nstats/templates/l10nstats/tree_progress.html#L48 is one. no idea why https://github.com/peterbe/elmo/blob/feature%2Fexternalize-all-media/apps/l10nstats/templates/l10nstats/history.html#L53 is jumping around. Re the js style, I see the argument for creating even worse code by commenting out lines of a lengthy statement. As you can see, that doesn't make me like that code style. I'm slightly on the other side for jquery-style spaghetti code, i.e., if we're talking about js code statement sequences. But for variable declarations, frowny face. Re file locations, if we go for the non-app-centric way for now, we should still prefix them in media already. That should make going for staticfiles a much more mechanic patch.
(In reply to comment #12) > the border in > https://github.com/peterbe/elmo/blob/feature%2Fexternalize-all-media/apps/ > l10nstats/templates/l10nstats/tree_progress.html#L48 is one. > Tidied up. > no idea why > https://github.com/peterbe/elmo/blob/feature%2Fexternalize-all-media/apps/ > l10nstats/templates/l10nstats/history.html#L53 is jumping around. > This was floating around wrongly on -stage- too but I've fixed it. > Re the js style, I see the argument for creating even worse code by > commenting out lines of a lengthy statement. As you can see, that doesn't > make me like that code style. I'm slightly on the other side for > jquery-style spaghetti code, i.e., if we're talking about js code statement > sequences. But for variable declarations, frowny face. > In insist, that style is quite common. The alternative is this: var foo = "test"; var bar = true; var etc; or this: var really_long_variable_name = "test", also_really_long = true, over_80_character_wide; I think the original reason why people use the style I propose is because it prevents you from accidentally having a trailing comma. Instead of commenting out, people might delete a line. > Re file locations, if we go for the non-app-centric way for now, we should > still prefix them in media already. That should make going for staticfiles a > much more mechanic patch. What I've done is started reorganizing static files a bit more per app. For example, I have this in settings/base.py now: 'bundles/l10nstats.l10nstats': ('js/l10nstats/l10nstats.js',), 'bundles/l10nstats.history': ('js/l10nstats/history.js',), 'bundles/l10nstats.tree_progress': ('js/l10nstats/tree_progress.js',), 'bundles/privacy.versions': ('js/privacy/versions.js',), Please put a couple of more eyes on the feature branch https://github.com/peterbe/elmo/tree/feature%2Fexternalize-all-media https://github.com/peterbe/elmo/tree/feature%2Fexternalize-all-media
Blocks: 662303
So, it's been agreed. We'll use django's staticfiles for all static resources and we'll deal with bundling/compression afterwards (see https://bugzilla.mozilla.org/show_bug.cgi?id=662303)
Blocks: 662541
Also major change is that jQuery is loaded in the very bottom of the DOM tree.
Attachment #537999 - Flags: review?(l10n)
Comment on attachment 537999 [details] [diff] [review] embedded javascript/css into .js/.css files Review of attachment 537999 [details] [diff] [review]: ----------------------------------------------------------------- This patch was a monster to write, I guess, and it is a monster to review. Navigation changes, code copies, code style changes, changes in error handling, changes in test style, and your editor being a whitespace n??i. For various of the comments below, this is an r- right now. In particular the licensing stuff applies at quite a few places, and the signoff1 ripout needs to be more thorough. style.css is up for debate, IMHO. Once this has an r+, we'll need to land this quickly after a release push and make sure we have a good week of pounding at it on -dev-. Splinter didn't offer me the following: apps/privacy/templates/privacy/shared.css: +delete me ::: .gitignore @@ +4,4 @@ > *.pyc > .coverage > cover/ > +apps/homepage/management/ This looks like leftovers? ::: apps/bugsy/static/bugsy/js/bugcount.js @@ +33,5 @@ > + * the provisions above, a recipient may use your version of this file under > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * Alternatively, the contents of this file may be used under the terms of > + * the MIT license. Don't change the license on existing code. Also, this project got asked to use triple-license by the foundation, and we're sticking to it (at least until MPL2 comes). This license header showed up a few more times down the patch. ::: apps/bugsy/templates/bugsy/new-locale.html @@ +55,2 @@ > </script> > +{{ FOO_URL }} Interesting, but probably not intended? ::: apps/bugsy/templates/bugsy/team-snippet.html @@ +55,4 @@ > }); > })(); > </script> > +{% endcomment %} Remove the code instead of comenting it out? The modularity of that page design really hurts us now. Let's see if that survives the wireframes. ::: apps/commons/tests/__init__.py @@ +32,5 @@ > +# and other provisions required by the GPL or the LGPL. If you do not delete > +# the provisions above, a recipient may use your version of this file under > +# the terms of any one of the MPL, the GPL or the LGPL. > +# > +# ***** END LICENSE BLOCK ***** No license headers on empty files, please. ::: apps/l10nstats/templates/l10nstats/tree_progress.html @@ +49,5 @@ > +<link rel="stylesheet" href="{{ STATIC_URL }}css/jquery.ui/base/ui.slider.css" type="text/css"> > +<link rel="stylesheet" href="{{ STATIC_URL }}css/jquery.ui/base/ui.tabs.css" type="text/css"> > +<link rel="stylesheet" href="{{ STATIC_URL }}css/jquery.ui/base/ui.datepicker.css" type="text/css"> > +<link rel="stylesheet" href="{{ STATIC_URL }}css/jquery.ui/base/ui.progressbar.css" type="text/css"> > +<link rel="stylesheet" href="{{ STATIC_URL }}css/jquery.ui/base/ui.theme.css" type="text/css"> How brittle is this when we take jquery.ui updates? Same for the script tags below. Also, is this taking a step back in loading files knowing that you want to bundle more later? In my head, I'm making different rules for our content and for the content included from other libraries. @@ +80,5 @@ > + , START_TIME_U = {{ startTime|date:"U" }} > + , END_TIME_U = {{ endTime|date:"U" }} > + , ALL_START_U = {{ allStart|date:"U" }} > + , ALL_END_U = {{ allEnd|date:"U" }} > +; I'll let this go by. I still think it's ugly as hell, and I'm not migrating my coding style to it. But I know it's there, so it doesn't look outright broken at this point. ::: apps/privacy/static/privacy/css/add.css @@ +11,5 @@ > + height: 2em; > +} > +.pending { > + opacity: 0.7; > +} This and the other statics in privacy need license headers still. ::: apps/privacy/views.py @@ +131,4 @@ > def post_policy(request): > if not (request.user.has_perm('privacy.add_policy') > and request.user.has_perm('privacy.add_comment')): > + return HttpResponseForbidden("not sufficient permissions") Why this? This also seems to be rather inconsistent now, some permission checks redirect, some don't. ::: apps/shipping/static/shipping/js/pushes.js @@ +1,1 @@ > +/* ***** BEGIN LICENSE BLOCK ***** I think this is signoff1. A merge conflict, the whole file should be gone? ::: build.py @@ +1,2 @@ > +BUILD_ID_CSS = "2c95a52" > +BUILD_ID_JS = "2c95a52" Not sure about these, leftovers? ::: static/css/style.css @@ +130,5 @@ > +} > +/* end Shipping pages */ > + > + > +/* pushes */ signoff1 stuff, most of this should be removed, AFAICT. @@ +332,5 @@ > +table#policy_versions th, table#policy_versions td { > + border: solid black 1px; > + padding: 2px; > +} > +/* end Privacy */ In general, I'm not really fond to have this pseudo bundle for the css contents. ::: urls.py @@ +48,5 @@ > # the references to /media/. > > # Remove leading and trailing slashes so the regex matches. > +# XXX: consider subclassing django.views.static.serve with something > +# that prints a warning message Is that XXX to stay for later or was that WIP? @@ +57,5 @@ > 'static'), > ) > > +#if settings.DEBUG: > +urlpatterns += staticfiles_urlpatterns() What's the #if for?
Attachment #537999 - Flags: review?(l10n) → review-
(In reply to comment #16) > Comment on attachment 537999 [details] [diff] [review] [review] > embedded javascript/css into .js/.css files > > Review of attachment 537999 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > This patch was a monster to write, I guess, and it is a monster to review. > Navigation changes, code copies, code style changes, changes in error > handling, changes in test style, and your editor being a whitespace n??i. > > For various of the comments below, this is an r- right now. In particular > the licensing stuff applies at quite a few places, and the signoff1 ripout > needs to be more thorough. style.css is up for debate, IMHO. > > Once this has an r+, we'll need to land this quickly after a release push > and make sure we have a good week of pounding at it on -dev-. > > Splinter didn't offer me the following: > > apps/privacy/templates/privacy/shared.css: > +delete me > Correct. It was deleted. Or rather, moved to ./apps/privacy/static/privacy/css/shared.css > ::: .gitignore > @@ +4,4 @@ > > *.pyc > > .coverage > > cover/ > > +apps/homepage/management/ > > This looks like leftovers? > Yep. That was something temporary I was working on. Removed. > ::: apps/bugsy/static/bugsy/js/bugcount.js > @@ +33,5 @@ > > + * the provisions above, a recipient may use your version of this file under > > + * the terms of any one of the MPL, the GPL or the LGPL. > > + * > > + * Alternatively, the contents of this file may be used under the terms of > > + * the MIT license. > > Don't change the license on existing code. Also, this project got asked to > use triple-license by the foundation, and we're sticking to it (at least > until MPL2 comes). > > This license header showed up a few more times down the patch. > So, for the bugsy app, the lines to remove are: " * Alternatively, the contents of this file may be used under the terms of * the MIT license. " Is that correct? If so, all fixed now. > ::: apps/bugsy/templates/bugsy/new-locale.html > @@ +55,2 @@ > > </script> > > +{{ FOO_URL }} > > Interesting, but probably not intended? > Debugging junk deleted now. > ::: apps/bugsy/templates/bugsy/team-snippet.html > @@ +55,4 @@ > > }); > > })(); > > </script> > > +{% endcomment %} > > Remove the code instead of comenting it out? > Commented out code removed. > The modularity of that page design really hurts us now. Let's see if that > survives the wireframes. > What I'm trying to achieve is to not have snippets that bundle javascript with html. That's just un-modulary design. It's better to bite the bullet and live with having to put in the right .js dependencies when using these snippets. Yes, I'm sure this will need another reorganization once we move towards the wireframes. > ::: apps/commons/tests/__init__.py > @@ +32,5 @@ > > +# and other provisions required by the GPL or the LGPL. If you do not delete > > +# the provisions above, a recipient may use your version of this file under > > +# the terms of any one of the MPL, the GPL or the LGPL. > > +# > > +# ***** END LICENSE BLOCK ***** > > No license headers on empty files, please. > Fixed. > ::: apps/l10nstats/templates/l10nstats/tree_progress.html > @@ +49,5 @@ > > +<link rel="stylesheet" href="{{ STATIC_URL }}css/jquery.ui/base/ui.slider.css" type="text/css"> > > +<link rel="stylesheet" href="{{ STATIC_URL }}css/jquery.ui/base/ui.tabs.css" type="text/css"> > > +<link rel="stylesheet" href="{{ STATIC_URL }}css/jquery.ui/base/ui.datepicker.css" type="text/css"> > > +<link rel="stylesheet" href="{{ STATIC_URL }}css/jquery.ui/base/ui.progressbar.css" type="text/css"> > > +<link rel="stylesheet" href="{{ STATIC_URL }}css/jquery.ui/base/ui.theme.css" type="text/css"> > > How brittle is this when we take jquery.ui updates? Same for the script tags > below. > Because I know we're going to use django_compressor later, this will become "/css/jquery.ui/1293812094813.css" when bundled. Or even better, we'll use the bundles that jQuery UI can make so that we always can be certain to have the right versions together. django_compressor looks at the original and makes bundles based on an MD5 of that. Therefore, if we, on one page include "foo.css, bar.css" on one page and "bar.css, box.css" on another we'll have to create two different bundles thus duplicating 'bar.css'. It's therefore better to create one bundle of "foo.css, bar.css, box.css". Makes sense? > Also, is this taking a step back in loading files knowing that you want to > bundle more later? In my head, I'm making different rules for our content > and for the content included from other libraries. > With regards to jQuery UI we ought to use the bundles you can make on the jQuery UI download page. They're complete, version numbered and we avoid the risk of accidentally missing a component. http://jqueryui.com/download With the (future) aggressive caching and unique URLs in production once the jQuery UI CSS or JS is downloaded it will work universally for all pages. > @@ +80,5 @@ > > + , START_TIME_U = {{ startTime|date:"U" }} > > + , END_TIME_U = {{ endTime|date:"U" }} > > + , ALL_START_U = {{ allStart|date:"U" }} > > + , ALL_END_U = {{ allEnd|date:"U" }} > > +; > > I'll let this go by. I still think it's ugly as hell, and I'm not migrating > my coding style to it. But I know it's there, so it doesn't look outright > broken at this point. > I assure you, it's quite common in JavaScript to write like this. It's not a "Peter thing". If you want to we can raise the discussion with the webdev team. > ::: apps/privacy/static/privacy/css/add.css > @@ +11,5 @@ > > + height: 2em; > > +} > > +.pending { > > + opacity: 0.7; > > +} > > This and the other statics in privacy need license headers still. > Hopefully all fixed now. I wrote a script called license_check.py which I'm adding in. It still reports some files on files I didn't create. Might file a new bug for that. > ::: apps/privacy/views.py > @@ +131,4 @@ > > def post_policy(request): > > if not (request.user.has_perm('privacy.add_policy') > > and request.user.has_perm('privacy.add_comment')): > > + return HttpResponseForbidden("not sufficient permissions") > > Why this? This also seems to be rather inconsistent now, some permission > checks redirect, some don't. > I'm trying to be consistent with this pattern: if POST: if not permission: raise Forbidden if GET: if not permission: raise Redirect Might be a bug for the future to get this into all views. Ideally then as a decorator. > ::: apps/shipping/static/shipping/js/pushes.js > @@ +1,1 @@ > > +/* ***** BEGIN LICENSE BLOCK ***** > > I think this is signoff1. A merge conflict, the whole file should be gone? > You "think". I was conservatively nervous that's why I left it in. I didn't confidently understand what you ripped out when you did away with signoff1. Would you mind taking another closer look? > ::: build.py > @@ +1,2 @@ > > +BUILD_ID_CSS = "2c95a52" > > +BUILD_ID_JS = "2c95a52" > > Not sure about these, leftovers? > Yep. From jingo_minify. Gone! > ::: static/css/style.css > @@ +130,5 @@ > > +} > > +/* end Shipping pages */ > > + > > + > > +/* pushes */ > > signoff1 stuff, most of this should be removed, AFAICT. > Gone! Thankfully git-grep helped me softly check them all. > @@ +332,5 @@ > > +table#policy_versions th, table#policy_versions td { > > + border: solid black 1px; > > + padding: 2px; > > +} > > +/* end Privacy */ > > In general, I'm not really fond to have this pseudo bundle for the css > contents. > Fair point. I'm not a huge fan either. I talked to wenzel and jbalogh about it and they said they favor putting as much as possible into style.css instead of individual files. However, I've reviewed it and found that there's lots of bits and pieces of CSS in the privacy app. So instead I cleaned it up and wrote just one file: apps/privacy/static/privacy/css/privacy.css > ::: urls.py > @@ +48,5 @@ > > # the references to /media/. > > > > # Remove leading and trailing slashes so the regex matches. > > +# XXX: consider subclassing django.views.static.serve with something > > +# that prints a warning message > > Is that XXX to stay for later or was that WIP? > Changed 'XXX' to 'TODO'. Will file a new bug for this. > @@ +57,5 @@ > > 'static'), > > ) > > > > +#if settings.DEBUG: > > +urlpatterns += staticfiles_urlpatterns() > > What's the #if for? We should never rely on staticfiles_urlpatterns() in production mode but because all of these changes I'd rather err on the side of caution. I've removed this particular comment and will instead focus on a wrapped 'django.views.static.serve' that spits out warning messages. Or some sort of carefully deprecating solution like that. Thanks for noticing.
See comment just before. This is a rebased patch that includes all those mentioned fixes and changes.
Attachment #537999 - Attachment is obsolete: true
Attachment #538477 - Flags: review?(l10n)
(In reply to comment #18) > Created attachment 538477 [details] [diff] [review] [review] > Patch that includes all fixes on Pike's review > > See comment just before. This is a rebased patch that includes all those > mentioned fixes and changes. Not included in the patch is that the commented out Privacy stuff in static/css/style.css has now been deleted on my local branch.
Comment on attachment 538477 [details] [diff] [review] Patch that includes all fixes on Pike's review I'm looking deeper, and find more things. Sadly, I find old things, too. There are still license headers in this patch that are MIT. Also, why do all new license headers claim that the original code is django-site pushes application? team.js has 3 space indention? l10nstats/js/index.js has a console.log, also 3 space indention. I won't finish going through the diff of all previously one and now distinct files, it's PTO for me after all. I came down to index.js, reading through the patch, and then using kdiff3 to look at each new file against the old html file on develop. I've asked to make code style to be a topic of today's elmo call.
Issues addressed: * All preambles corrected. The .js and .css files now match the templates the came from original. I.e. right year and right "Original Code ..." bit * All .js files tidied up with 2 spaces indentation. Everywhere. All trivial whitespace corrections applied (e.g. 'function (a,b){ return b}` ==> 'function (a, b) { return b; }`. * The update_site.py script has been updated to execute `./manage.py collectstatic --noinput` (we'll need to correct our apache config once we deploy but there's not super rush since the slow and insecure urlpattern trick in staticfiles works even when in DEBUG=False) * All embedded CSS and Javascript in the signoffs2 has been externalized. * Two more tests added that check static files rendering related to milestones (in shipping app) * The pushes.js remnant from ripping out signoff1 has been removed now. * Removed the accidentally left over 'console.log` statement.
Attachment #538477 - Attachment is obsolete: true
Attachment #539482 - Flags: review?(l10n)
Attachment #538477 - Flags: review?(l10n)
Blocks: 662817
The only two changes in this updated patch are: * removed accidentally left debugging statement in diff.html ('{% load fooo %}') * corrected license preamble in static/js/base.js
Attachment #539482 - Attachment is obsolete: true
Attachment #540053 - Flags: review?(l10n)
Attachment #539482 - Flags: review?(l10n)
Comment on attachment 540053 [details] [diff] [review] Removed missed debugging statement in diff.html Review of attachment 540053 [details] [diff] [review]: ----------------------------------------------------------------- This is getting really close. It'd likely have an r+ by now if it wasn't trying to hit a moving target. I managed to do a full review pass, and I'm pretty sure there's nothing I missed, so addressing these comments should fix the thing. This is borderline between r+ with comments and r-, the permission change on confirm_ship_mstone made the call to an r- on this one. ::: apps/l10nstats/static/l10nstats/js/history.js @@ +43,5 @@ > + timeGeometry = new Timeplot.MagnifyingTimeGeometry({ > + gridColor: new Timeplot.Color('#000000'), > + axisLabelsPlacement: 'top' > + > + }); Mixed 2 and 4 spaces indention here, and throughout this file. ::: apps/l10nstats/static/l10nstats/js/tree_progress.js @@ +130,5 @@ > + theme.event.bubble.height = 50; > + localesSource = new Timeplot.DefaultEventSource(); > + changesSource = new Timeplot.DefaultEventSource(); > + var tgParams = { > + gridColor: new Timeplot.Color('#000000'), There's mixed indention in this file again. Not sure if we need to fix the multiple vars in one line that are still here, as well as the if () single_line(); ones. @@ +227,5 @@ > + if (loclist.length) { > + loclist.sort(); > + changeEvents.push(new E({start: loc_data[i].time, > + description: loclist.join(' '), > + instant: false})); put the start piece on its own line, too, so that things align again? @@ +232,5 @@ > + } > + locEvents.push(new NE(loc_data[i].time, state.data())); > + } > + locEvents.push(new NE(SimileAjax.DateTime > + .parseIso8601DateTime(END_TIME), state.data())); Can we wrap this line in a way that one can still read it? locEvents.push( new NE( SimileAjax.DateTime.parseIso8601DateTime(END_TIME), state.data() ) ); or so? ::: apps/l10nstats/templates/l10nstats/tree_progress.html @@ +80,5 @@ > + , START_TIME_U = {{ startTime|date:"U" }} > + , END_TIME_U = {{ endTime|date:"U" }} > + , ALL_START_U = {{ allStart|date:"U" }} > + , ALL_END_U = {{ allEnd|date:"U" }} > +; Do the one-var-per-line for these? ::: apps/privacy/static/privacy/js/versions.js @@ +41,5 @@ > + buttons: { > + 'Cancel': function() { > + $(this).dialog('close'); > + }, > + 'OK': function() { I think style-guide says to actually quote neither Cancel nor OK. ::: apps/pushes/static/pushes/js/pushlog.js @@ +77,5 @@ > + $(this).addClass('ui-state-focus'); > + }) > + .blur(function() { > + $(this).removeClass('ui-state-focus'); > + }); This turned out unreadable, IMHO. $('#search') .click(function() { d.dialog('open'); }) .hover( function() { $(this).addClass('ui-state-hover'); }, function() { $(this).removeClass('ui-state-hover'); }) .focus(function() { $(this).addClass('ui-state-focus'); }) .blur(function() { $(this).removeClass('ui-state-focus'); }); conveys the message much better. I'm intentionally inconsistent in whether function() is on the line of the jquery method or not, in a way. For a single callback, I'm on the same line, as that's easy to read. For hover, which takes two callbacks, I put in more structure so that the two callbacks are obviously two callbacks. Also, jquery sequences that all work on the same query should stay in one indention level. @@ +131,5 @@ > + $('#searchrows').append('<tr><td>Path part</td><td><input name="path" type="text" size="10" maxlength="30"></tr>'); > + break; > + case 'search_repos': > + $('#searchrows').append('<tr><td>Repository part</td><td><input name="repo" type="text" size="10" maxlength="30"></tr>'); > + break; 2-space indention, please. The style-guide isn't explicit on this, but the node sourcecode is. ::: media/js/widgets.js @@ -34,5 @@ > * the terms of any one of the MPL, the GPL or the LGPL. > * > - * Alternatively, the contents of this file may be used under the terms of > - * the MIT license. > - * This is relicensing an existing file, not good. Background, this code is also under MIT license so that it'd be compatible with jquery.ui licensing, if it was successful and wanted to be liberated. ::: apps/privacy/templates/privacy/shared.css @@ +39,5 @@ > + $('.ui-accordion-header').click(function(e) { > + $(this).each(function(i) { > + $(this).children('.ui-icon') > + .toggleClass('ui-icon-triangle-1-s') > + .toggleClass('ui-icon-triangle-1-e'); This is in diff.js (git diff got confused): Please no extra indent for jquery sequences. ::: apps/shipping/static/shipping/js/signoffs.js @@ +42,5 @@ > +} > + > +$(document).ready(function() { > + $('.diffanchor').draggable({ > + appendTo: 'body', mixed indention @@ +76,5 @@ > + function hoverSO(showOrHide) { > + return function() { > + var q = $('#so_' + this.getAttribute('data-push')) > + .not('.suggested') > + .not('.clicked'); no extra indent please. ::: apps/shipping/tests.py @@ +43,4 @@ > from django.conf import settings > from shipping.models import Milestone, Application, AppVersion, Signoff, Action > from shipping.views import _signoffs > +from life.models import Tree, Forest, Locale, Repository Repository is unused? ::: apps/shipping/views/__init__.py @@ +447,5 @@ > Redirects to milestones() in case of trouble. > """ > + if not request.GET.get('ms'): > + raise Http404("ms must be supplied") > + if not request.user.has_perm('shipping.can_ship'): Please don't make confirm_ship_mstone protected. If you need it to be consistent with confirm_drill_mstone, remove the protection there. ::: apps/tinder/templates/tinder/tbpl.html @@ +49,5 @@ > +.stamp {clear: left; } > +.changes {background-color: lightgrey; padding: 0.25em;} > +.who {background-color: lightgrey; padding: 0.25em;} > +.top {-moz-border-radius-topleft: 10px} > +.bottom {-moz-border-radius-bottomleft: 10px} There's a good deal of overlap between this and waterfall.css. This should be one CSS file for both, I'd say. Colors either from waterfall, or the suggestions in bug 579075. I'd leave those up for that bug, though. ::: static/css/style.css @@ +50,5 @@ > +} > +/* end Teams page */ > + > + > +/* Shipping pages */ The shipping pages stuff can go. The fieldset stuff was solely signoff1, and the rest is really just affecting /shipping/, which looks better without the stuff here. Remove from here to ... @@ +127,5 @@ > + border-top: 1px solid #990000; > + border-left: 1px solid #990000; > + border-right: 1px solid #990000; > +} > +/* end Shipping pages */ .. here. @@ +131,5 @@ > +/* end Shipping pages */ > + > + > +/* l10nstats */ > +.exhibit-facet-body {height: 5em !important;} This doesn't belong in a site-wide stylesheet. @@ +135,5 @@ > +.exhibit-facet-body {height: 5em !important;} > +div.thumbnail {border: solid black 1px; margin: 2px;} > +.success {background-color: green;} > +.warning {background-color: orange;} > +.failure {background-color: red;} These conflict with waterfall.css. @@ +177,5 @@ > +} > +.bugfield_short_desc { > + padding-left: 1em; > +} > +/* end l10nstats */ The two comments above make me think that the l10nstats CSS rules should go into one shared CSS file in that app.
Attachment #540053 - Flags: review?(l10n) → review-
(In reply to comment #24) > Comment on attachment 540053 [details] [diff] [review] [review] > Removed missed debugging statement in diff.html > > Review of attachment 540053 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > This is getting really close. It'd likely have an r+ by now if it wasn't > trying to hit a moving target. > > I managed to do a full review pass, and I'm pretty sure there's nothing I > missed, so addressing these comments should fix the thing. > > This is borderline between r+ with comments and r-, the permission change on > confirm_ship_mstone made the call to an r- on this one. > > ::: apps/l10nstats/static/l10nstats/js/history.js > @@ +43,5 @@ > > + timeGeometry = new Timeplot.MagnifyingTimeGeometry({ > > + gridColor: new Timeplot.Color('#000000'), > > + axisLabelsPlacement: 'top' > > + > > + }); > > Mixed 2 and 4 spaces indention here, and throughout this file. > Fixed. > ::: apps/l10nstats/static/l10nstats/js/tree_progress.js > @@ +130,5 @@ > > + theme.event.bubble.height = 50; > > + localesSource = new Timeplot.DefaultEventSource(); > > + changesSource = new Timeplot.DefaultEventSource(); > > + var tgParams = { > > + gridColor: new Timeplot.Color('#000000'), > > There's mixed indention in this file again. > Fixed. > Not sure if we need to fix the multiple vars in one line that are still > here, as well as the if () single_line(); ones. > Where? what? We're not doing multi-line variable declarations there. > @@ +227,5 @@ > > + if (loclist.length) { > > + loclist.sort(); > > + changeEvents.push(new E({start: loc_data[i].time, > > + description: loclist.join(' '), > > + instant: false})); > > put the start piece on its own line, too, so that things align again? > Fixed. > @@ +232,5 @@ > > + } > > + locEvents.push(new NE(loc_data[i].time, state.data())); > > + } > > + locEvents.push(new NE(SimileAjax.DateTime > > + .parseIso8601DateTime(END_TIME), state.data())); > > Can we wrap this line in a way that one can still read it? > > locEvents.push( > new NE( > SimileAjax.DateTime.parseIso8601DateTime(END_TIME), > state.data() > ) > ); > > or so? > Fixed. > ::: apps/l10nstats/templates/l10nstats/tree_progress.html > @@ +80,5 @@ > > + , START_TIME_U = {{ startTime|date:"U" }} > > + , END_TIME_U = {{ endTime|date:"U" }} > > + , ALL_START_U = {{ allStart|date:"U" }} > > + , ALL_END_U = {{ allEnd|date:"U" }} > > +; > > Do the one-var-per-line for these? > Fixed. > ::: apps/privacy/static/privacy/js/versions.js > @@ +41,5 @@ > > + buttons: { > > + 'Cancel': function() { > > + $(this).dialog('close'); > > + }, > > + 'OK': function() { > > I think style-guide says to actually quote neither Cancel nor OK. > Fixed. > ::: apps/pushes/static/pushes/js/pushlog.js > @@ +77,5 @@ > > + $(this).addClass('ui-state-focus'); > > + }) > > + .blur(function() { > > + $(this).removeClass('ui-state-focus'); > > + }); > > This turned out unreadable, IMHO. > > $('#search') > .click(function() { > d.dialog('open'); > }) > .hover( > function() { > $(this).addClass('ui-state-hover'); > }, > function() { > $(this).removeClass('ui-state-hover'); > }) > .focus(function() { > $(this).addClass('ui-state-focus'); > }) > .blur(function() { > $(this).removeClass('ui-state-focus'); > }); > > conveys the message much better. > > I'm intentionally inconsistent in whether function() is on the line of the > jquery method or not, in a way. For a single callback, I'm on the same line, > as that's easy to read. For hover, which takes two callbacks, I put in more > structure so that the two callbacks are obviously two callbacks. > > Also, jquery sequences that all work on the same query should stay in one > indention level. I think that's ugly. The closing on the 'click' callback isn't aligned with where it started. I did a little improvement to the code anyway but I don't think we should shift the indentation "during" a block. > > @@ +131,5 @@ > > + $('#searchrows').append('<tr><td>Path part</td><td><input name="path" type="text" size="10" maxlength="30"></tr>'); > > + break; > > + case 'search_repos': > > + $('#searchrows').append('<tr><td>Repository part</td><td><input name="repo" type="text" size="10" maxlength="30"></tr>'); > > + break; > > 2-space indention, please. The style-guide isn't explicit on this, but the > node sourcecode is. > Fixed. > ::: media/js/widgets.js > @@ -34,5 @@ > > * the terms of any one of the MPL, the GPL or the LGPL. > > * > > - * Alternatively, the contents of this file may be used under the terms of > > - * the MIT license. > > - * > > This is relicensing an existing file, not good. > > Background, this code is also under MIT license so that it'd be compatible > with jquery.ui licensing, if it was successful and wanted to be liberated. > OK. I've put it back into widgets.js > ::: apps/privacy/templates/privacy/shared.css > @@ +39,5 @@ > > + $('.ui-accordion-header').click(function(e) { > > + $(this).each(function(i) { > > + $(this).children('.ui-icon') > > + .toggleClass('ui-icon-triangle-1-s') > > + .toggleClass('ui-icon-triangle-1-e'); > > This is in diff.js (git diff got confused): > > Please no extra indent for jquery sequences. > Why not? This is the correct way of doing things. It's a style guide relic from C. jQuery happens to use chaining mutually exclusive but in general if you do chaining the order matters so therefore the indentation needs to be there. > ::: apps/shipping/static/shipping/js/signoffs.js > @@ +42,5 @@ > > +} > > + > > +$(document).ready(function() { > > + $('.diffanchor').draggable({ > > + appendTo: 'body', > > mixed indention > Fixed. > @@ +76,5 @@ > > + function hoverSO(showOrHide) { > > + return function() { > > + var q = $('#so_' + this.getAttribute('data-push')) > > + .not('.suggested') > > + .not('.clicked'); > > no extra indent please. > Again, see point above about the risks about chaining and order dependency. > ::: apps/shipping/tests.py > @@ +43,4 @@ > > from django.conf import settings > > from shipping.models import Milestone, Application, AppVersion, Signoff, Action > > from shipping.views import _signoffs > > +from life.models import Tree, Forest, Locale, Repository > > Repository is unused? > This got me running check.py on a bunch of tests.py files (almost all of them written by me at some point) and got slightly side-tracked so I changed some other files. Sorry. We can make a new bug about fixing ALL flakes and pep8isms at a later stage. > ::: apps/shipping/views/__init__.py > @@ +447,5 @@ > > Redirects to milestones() in case of trouble. > > """ > > + if not request.GET.get('ms'): > > + raise Http404("ms must be supplied") > > + if not request.user.has_perm('shipping.can_ship'): > > Please don't make confirm_ship_mstone protected. > Why? This is a very common pattern. If you don't have the permission you're giving the user a "false sense of hope". Also, having a "user friendly" redirect in there prevents us from having to raise the 403 error on the POST later. > If you need it to be consistent with confirm_drill_mstone, remove the > protection there. > > ::: apps/tinder/templates/tinder/tbpl.html > @@ +49,5 @@ > > +.stamp {clear: left; } > > +.changes {background-color: lightgrey; padding: 0.25em;} > > +.who {background-color: lightgrey; padding: 0.25em;} > > +.top {-moz-border-radius-topleft: 10px} > > +.bottom {-moz-border-radius-bottomleft: 10px} > > There's a good deal of overlap between this and waterfall.css. This should > be one CSS file for both, I'd say. Colors either from waterfall, or the > suggestions in bug 579075. I'd leave those up for that bug, though. > I created tbpl.css sitting next to waterfall.css > ::: static/css/style.css > @@ +50,5 @@ > > +} > > +/* end Teams page */ > > + > > + > > +/* Shipping pages */ > > The shipping pages stuff can go. The fieldset stuff was solely signoff1, and > the rest is really just affecting /shipping/, which looks better without the > stuff here. > > Remove from here to ... > > @@ +127,5 @@ > > + border-top: 1px solid #990000; > > + border-left: 1px solid #990000; > > + border-right: 1px solid #990000; > > +} > > +/* end Shipping pages */ > > .. here. > Sorry, that's not clear. Can you file a new bug for that? > @@ +131,5 @@ > > +/* end Shipping pages */ > > + > > + > > +/* l10nstats */ > > +.exhibit-facet-body {height: 5em !important;} > > This doesn't belong in a site-wide stylesheet. > Why? Aren't these rules applicable to a bunch of l10nstats pages? As long as it's commented there's nothing wrong with putting app specific CSS in the "global" one. > @@ +135,5 @@ > > +.exhibit-facet-body {height: 5em !important;} > > +div.thumbnail {border: solid black 1px; margin: 2px;} > > +.success {background-color: green;} > > +.warning {background-color: orange;} > > +.failure {background-color: red;} > > These conflict with waterfall.css. > Do they? I can't see that. Can we raise a new bug for that? I feel like this bug is straying into something too different from the bug topic which means it will continue to block. > @@ +177,5 @@ > > +} > > +.bugfield_short_desc { > > + padding-left: 1em; > > +} > > +/* end l10nstats */ > > The two comments above make me think that the l10nstats CSS rules should go > into one shared CSS file in that app. There's lots of l10nstats pages. We don't really have a better way to define whole-app specific CSS.
See previous in-line comments.
Attachment #540053 - Attachment is obsolete: true
Attachment #541127 - Flags: review?(l10n)
Following up on the discussion in the elmo call, let's get the notes and my offline comments into the bug: (In reply to comment #25) > (In reply to comment #24) > > Comment on attachment 540053 [details] [diff] [review] [review] [review] > > Removed missed debugging statement in diff.html > > > > Review of attachment 540053 [details] [diff] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > This is getting really close. It'd likely have an r+ by now if it wasn't > > trying to hit a moving target. > > > > I managed to do a full review pass, and I'm pretty sure there's nothing I > > missed, so addressing these comments should fix the thing. > > > > This is borderline between r+ with comments and r-, the permission change on > > confirm_ship_mstone made the call to an r- on this one. > > > > ::: apps/l10nstats/static/l10nstats/js/history.js > > @@ +43,5 @@ > > > + timeGeometry = new Timeplot.MagnifyingTimeGeometry({ > > > + gridColor: new Timeplot.Color('#000000'), > > > + axisLabelsPlacement: 'top' > > > + > > > + }); > > > > Mixed 2 and 4 spaces indention here, and throughout this file. > > > Fixed. There's more awkward indenting in history.js still: + var valueGeometry = new Timeplot.DefaultValueGeometry({ + gridColor: '#000000', + min: 0, + axisLabelsPlacement: 'left' + }); 2 + 3 spaces + var valueGeometry2 = new Timeplot.DefaultValueGeometry({ + gridColor: '#000000', + min: 0 + }); 2 + 4 spaces + var plotInfo = [ + Timeplot.createPlotInfo({ + id: 'checkins', + timeGeometry: timeGeometry, + eventSource: eventSource2, + lineColor: 'blue' + }), 4 + 2 spaces. Which is why I said "throughout the file". > > ::: apps/l10nstats/static/l10nstats/js/tree_progress.js > > @@ +130,5 @@ > > > + theme.event.bubble.height = 50; > > > + localesSource = new Timeplot.DefaultEventSource(); > > > + changesSource = new Timeplot.DefaultEventSource(); > > > + var tgParams = { > > > + gridColor: new Timeplot.Color('#000000'), > > > > There's mixed indention in this file again. > > > Fixed. > > > Not sure if we need to fix the multiple vars in one line that are still > > here, as well as the if () single_line(); ones. > > > Where? what? > We're not doing multi-line variable declarations there. multiple vars in one line like in + var v = 0, rv = {}, _d = this._data; Also, there are a bunch of + if ('earliestDate' in this) rv.earliestDate = this.earliestDate; or + if (params.showBad) + plotInfo.push(badPlot); Again, not sure if we need to bother about that now. <...> > > ::: apps/pushes/static/pushes/js/pushlog.js > > @@ +77,5 @@ > > > + $(this).addClass('ui-state-focus'); > > > + }) > > > + .blur(function() { > > > + $(this).removeClass('ui-state-focus'); > > > + }); > > > > This turned out unreadable, IMHO. > > > > $('#search') > > .click(function() { > > d.dialog('open'); > > }) > > .hover( > > function() { > > $(this).addClass('ui-state-hover'); > > }, > > function() { > > $(this).removeClass('ui-state-hover'); > > }) > > .focus(function() { > > $(this).addClass('ui-state-focus'); > > }) > > .blur(function() { > > $(this).removeClass('ui-state-focus'); > > }); > > > > conveys the message much better. > > > > I'm intentionally inconsistent in whether function() is on the line of the > > jquery method or not, in a way. For a single callback, I'm on the same line, > > as that's easy to read. For hover, which takes two callbacks, I put in more > > structure so that the two callbacks are obviously two callbacks. > > > > Also, jquery sequences that all work on the same query should stay in one > > indention level. > I think that's ugly. The closing on the 'click' callback isn't aligned with > where it started. > I did a little improvement to the code anyway but I don't think we should > shift the indentation "during" a block. As per elmo chat, we want to make this like: $('#search').click(function() { d.dialog('open'); }) .hover( function() { $(this).addClass('ui-state-hover'); }, function() { $(this).removeClass('ui-state-hover'); } ) .focus(function() { $(this).addClass('ui-state-focus'); }) .blur(function() { $(this).removeClass('ui-state-focus'); }); > > ::: apps/privacy/templates/privacy/shared.css > > @@ +39,5 @@ > > > + $('.ui-accordion-header').click(function(e) { > > > + $(this).each(function(i) { > > > + $(this).children('.ui-icon') > > > + .toggleClass('ui-icon-triangle-1-s') > > > + .toggleClass('ui-icon-triangle-1-e'); > > > > This is in diff.js (git diff got confused): > > > > Please no extra indent for jquery sequences. > > > Why not? This is the correct way of doing things. It's a style guide relic > from C. jQuery happens to use chaining mutually exclusive but in general if > you do chaining the order matters so therefore the indentation needs to be > there. As per elmo chat, the .toggleClass should be on the same indention level. > > ::: apps/shipping/static/shipping/js/signoffs.js > > @@ +42,5 @@ > > > +} > > > + > > > +$(document).ready(function() { > > > + $('.diffanchor').draggable({ > > > + appendTo: 'body', > > > > mixed indention > > > Fixed. > > > @@ +76,5 @@ > > > + function hoverSO(showOrHide) { > > > + return function() { > > > + var q = $('#so_' + this.getAttribute('data-push')) > > > + .not('.suggested') > > > + .not('.clicked'); > > > > no extra indent please. > > > Again, see point above about the risks about chaining and order dependency. This one is borderline to our discussion, as I just see. .not() returns a different set of nodes, and thus doesn't fall under the same argument that .eventhandler() would. OTH, .not().not() isn't order-dependent. I'd favor same indention level for both .not, but can go either way. <...> > > ::: apps/shipping/views/__init__.py > > @@ +447,5 @@ > > > Redirects to milestones() in case of trouble. > > > """ > > > + if not request.GET.get('ms'): > > > + raise Http404("ms must be supplied") > > > + if not request.user.has_perm('shipping.can_ship'): > > > > Please don't make confirm_ship_mstone protected. > > > Why? This is a very common pattern. If you don't have the permission you're > giving the user a "false sense of hope". > Also, having a "user friendly" redirect in there prevents us from having to > raise the 403 error on the POST later. As per elmo meeting, the permission check should be reverted. <...> > > @@ +131,5 @@ > > > +/* end Shipping pages */ > > > + > > > + > > > +/* l10nstats */ > > > +.exhibit-facet-body {height: 5em !important;} > > > > This doesn't belong in a site-wide stylesheet. > > > Why? Aren't these rules applicable to a bunch of l10nstats pages? > As long as it's commented there's nothing wrong with putting app specific > CSS in the "global" one. The .exhibit-facet-body is automatically generated by all exhibits, and that rule was only supposed to apply on that one page. There's no indication that this would be a good site-wide setting. > > @@ +135,5 @@ > > > +.exhibit-facet-body {height: 5em !important;} > > > +div.thumbnail {border: solid black 1px; margin: 2px;} > > > +.success {background-color: green;} > > > +.warning {background-color: orange;} > > > +.failure {background-color: red;} > > > > These conflict with waterfall.css. > > > Do they? I can't see that. waterfall.css says: +.success {background-color: rgba(0, 128, 0, 0.5);} style.css says: (as does tbpl.css) +.success {background-color: green;}
Probably for a follow-up bug, but I stumble upon http://weblogs.mozillazine.org/roc/archives/2011/06/auckland_web_me_4.html, in its third paragraph, roc says: ... Most of the suggestions were good, but there was some advice to put external script loads as late in the HTML page as possible. That probably made sense in older engines -- it let other resources start loading without having to wait for scripts to load. However, modern engines can speculatively parse past a <script src> element before it finishes loading, so these days it's probably best to put external script loads as early in the HTML page as possible.
Comment on attachment 541127 [details] [diff] [review] Fixes for the latest review comments r- based on comment 27.
Attachment #541127 - Flags: review?(l10n) → review-
(In reply to comment #27) > Following up on the discussion in the elmo call, let's get the notes and my > offline comments into the bug: > > (In reply to comment #25) > > (In reply to comment #24) > > > Comment on attachment 540053 [details] [diff] [review] [review] [review] [review] > > > Removed missed debugging statement in diff.html > > > > > > Review of attachment 540053 [details] [diff] [review] [review] [review] [review]: > > > ----------------------------------------------------------------- > > > > > > This is getting really close. It'd likely have an r+ by now if it wasn't > > > trying to hit a moving target. > > > > > > I managed to do a full review pass, and I'm pretty sure there's nothing I > > > missed, so addressing these comments should fix the thing. > > > > > > This is borderline between r+ with comments and r-, the permission change on > > > confirm_ship_mstone made the call to an r- on this one. > > > > > > ::: apps/l10nstats/static/l10nstats/js/history.js > > > @@ +43,5 @@ > > > > + timeGeometry = new Timeplot.MagnifyingTimeGeometry({ > > > > + gridColor: new Timeplot.Color('#000000'), > > > > + axisLabelsPlacement: 'top' > > > > + > > > > + }); > > > > > > Mixed 2 and 4 spaces indention here, and throughout this file. > > > > > Fixed. > > There's more awkward indenting in history.js still: > > + var valueGeometry = new Timeplot.DefaultValueGeometry({ > + gridColor: '#000000', > + min: 0, > + axisLabelsPlacement: 'left' > + }); > > 2 + 3 spaces > > + var valueGeometry2 = new Timeplot.DefaultValueGeometry({ > + gridColor: '#000000', > + min: 0 > + }); > > 2 + 4 spaces > > + var plotInfo = [ > + Timeplot.createPlotInfo({ > + id: 'checkins', > + timeGeometry: timeGeometry, > + eventSource: eventSource2, > + lineColor: 'blue' > + }), > > 4 + 2 spaces. Which is why I said "throughout the file". > Fixed. Note, indentation for things like arrays (e.g. 'var plotInfo = [...') is usually outside the "scope" of structural indentation. Even hardcore PEP8 nazis accept less strict indentation policies on things like big dicts or lists. > > > ::: apps/l10nstats/static/l10nstats/js/tree_progress.js > > > @@ +130,5 @@ > > > > + theme.event.bubble.height = 50; > > > > + localesSource = new Timeplot.DefaultEventSource(); > > > > + changesSource = new Timeplot.DefaultEventSource(); > > > > + var tgParams = { > > > > + gridColor: new Timeplot.Color('#000000'), > > > > > > There's mixed indention in this file again. > > > > > Fixed. > > > > > Not sure if we need to fix the multiple vars in one line that are still > > > here, as well as the if () single_line(); ones. > > > > > Where? what? > > We're not doing multi-line variable declarations there. > > multiple vars in one line like in > > + var v = 0, rv = {}, _d = this._data; > > Also, there are a bunch of > > + if ('earliestDate' in this) rv.earliestDate = this.earliestDate; > > or > > + if (params.showBad) > + plotInfo.push(badPlot); > > > Again, not sure if we need to bother about that now. > No biggies. The MUCH bigger fish to fry is the fact that "v", "rv" and "_d" are terrible variable names. Let's just remember; next time we write large chunks of Javascript...: 1) write tests 2) use descriptive variable names 3) avoid oneliners > <...> > > > > ::: apps/pushes/static/pushes/js/pushlog.js > > > @@ +77,5 @@ > > > > + $(this).addClass('ui-state-focus'); > > > > + }) > > > > + .blur(function() { > > > > + $(this).removeClass('ui-state-focus'); > > > > + }); > > > > > > This turned out unreadable, IMHO. > > > > > > $('#search') > > > .click(function() { > > > d.dialog('open'); > > > }) > > > .hover( > > > function() { > > > $(this).addClass('ui-state-hover'); > > > }, > > > function() { > > > $(this).removeClass('ui-state-hover'); > > > }) > > > .focus(function() { > > > $(this).addClass('ui-state-focus'); > > > }) > > > .blur(function() { > > > $(this).removeClass('ui-state-focus'); > > > }); > > > > > > conveys the message much better. > > > > > > I'm intentionally inconsistent in whether function() is on the line of the > > > jquery method or not, in a way. For a single callback, I'm on the same line, > > > as that's easy to read. For hover, which takes two callbacks, I put in more > > > structure so that the two callbacks are obviously two callbacks. > > > > > > Also, jquery sequences that all work on the same query should stay in one > > > indention level. > > I think that's ugly. The closing on the 'click' callback isn't aligned with > > where it started. > > I did a little improvement to the code anyway but I don't think we should > > shift the indentation "during" a block. > > As per elmo chat, we want to make this like: > > $('#search').click(function() { > d.dialog('open'); > }) > .hover( > function() { > $(this).addClass('ui-state-hover'); > }, > function() { > $(this).removeClass('ui-state-hover'); > } > ) > .focus(function() { > $(this).addClass('ui-state-focus'); > }) > .blur(function() { > $(this).removeClass('ui-state-focus'); > }); > Done. > > > ::: apps/privacy/templates/privacy/shared.css > > > @@ +39,5 @@ > > > > + $('.ui-accordion-header').click(function(e) { > > > > + $(this).each(function(i) { > > > > + $(this).children('.ui-icon') > > > > + .toggleClass('ui-icon-triangle-1-s') > > > > + .toggleClass('ui-icon-triangle-1-e'); > > > > > > This is in diff.js (git diff got confused): > > > > > > Please no extra indent for jquery sequences. > > > > > Why not? This is the correct way of doing things. It's a style guide relic > > from C. jQuery happens to use chaining mutually exclusive but in general if > > you do chaining the order matters so therefore the indentation needs to be > > there. > > As per elmo chat, the .toggleClass should be on the same indention level. > Done. > > > ::: apps/shipping/static/shipping/js/signoffs.js > > > @@ +42,5 @@ > > > > +} > > > > + > > > > +$(document).ready(function() { > > > > + $('.diffanchor').draggable({ > > > > + appendTo: 'body', > > > > > > mixed indention > > > > > Fixed. > > > > > @@ +76,5 @@ > > > > + function hoverSO(showOrHide) { > > > > + return function() { > > > > + var q = $('#so_' + this.getAttribute('data-push')) > > > > + .not('.suggested') > > > > + .not('.clicked'); > > > > > > no extra indent please. > > > > > Again, see point above about the risks about chaining and order dependency. > > This one is borderline to our discussion, as I just see. > > .not() returns a different set of nodes, and thus doesn't fall under the > same argument that .eventhandler() would. > OTH, .not().not() isn't order-dependent. > > I'd favor same indention level for both .not, but can go either way. > Going to leave it as is. Since the not selector order can matter. > <...> > > > > ::: apps/shipping/views/__init__.py > > > @@ +447,5 @@ > > > > Redirects to milestones() in case of trouble. > > > > """ > > > > + if not request.GET.get('ms'): > > > > + raise Http404("ms must be supplied") > > > > + if not request.user.has_perm('shipping.can_ship'): > > > > > > Please don't make confirm_ship_mstone protected. > > > > > Why? This is a very common pattern. If you don't have the permission you're > > giving the user a "false sense of hope". > > Also, having a "user friendly" redirect in there prevents us from having to > > raise the 403 error on the POST later. > > As per elmo meeting, the permission check should be reverted. > Done. And I have unit tests to prove it :) > <...> > > > > @@ +131,5 @@ > > > > +/* end Shipping pages */ > > > > + > > > > + > > > > +/* l10nstats */ > > > > +.exhibit-facet-body {height: 5em !important;} > > > > > > This doesn't belong in a site-wide stylesheet. > > > > > Why? Aren't these rules applicable to a bunch of l10nstats pages? > > As long as it's commented there's nothing wrong with putting app specific > > CSS in the "global" one. > > The .exhibit-facet-body is automatically generated by all exhibits, and that > rule was only supposed to apply on that one page. There's no indication that > this would be a good site-wide setting. > Feeling very dim right now. If class="exhibit-facet-body" is used on multiple pages isn't it good to have it in the global file? > > > @@ +135,5 @@ > > > > +.exhibit-facet-body {height: 5em !important;} > > > > +div.thumbnail {border: solid black 1px; margin: 2px;} > > > > +.success {background-color: green;} > > > > +.warning {background-color: orange;} > > > > +.failure {background-color: red;} > > > > > > These conflict with waterfall.css. > > > > > Do they? I can't see that. > > waterfall.css says: > +.success {background-color: rgba(0, 128, 0, 0.5);} > > style.css says: (as does tbpl.css) > +.success {background-color: green;} So, waterfall.css overrides with a different colour? What's wrong with that? Semantically I don't see what's wrong with it but from a user point of view if the colour for "success" is odd on one page.
Attached patch Indentation and permission fixes (obsolete) — — Splinter Review
No changes regarding the waterfall CSS. This bug was about moving embedded css and js, not changing permission logic or refactoring js/css so let's keep things simple.
Attachment #541127 - Attachment is obsolete: true
Attachment #542482 - Flags: review?(l10n)
(In reply to comment #28) > Probably for a follow-up bug, but I stumble upon > http://weblogs.mozillazine.org/roc/archives/2011/06/auckland_web_me_4.html, > in its third paragraph, roc says: > > ... Most of the suggestions were good, but there was some advice to put > external script loads as late in the HTML page as possible. That probably > made sense in older engines -- it let other resources start loading without > having to wait for scripts to load. However, modern engines can > speculatively parse past a <script src> element before it finishes loading, > so these days it's probably best to put external script loads as early in > the HTML page as possible. It's not just a matter of modern browsers. Slow computers struggle with the concurrency and potential bombardment on the garbage collector when dealing with the Javascript file. Getting the display right first is better. If you have something to look at you won't notice how the computer is struggling in the background. Most people don't have slow computers but they do have slow smartphones or average speed computers with 60 tabs open. Secondly, having the jQuery loaded last prevents messy markup like this: <input name="foo" onclick="ajax_post_something()"> or like this: <h1>Projects <script> $.getJSON('/countprojects', function(r) { document.write(r) }); </script> </h1>
(In reply to comment #30) <...> > > > > > > @@ +131,5 @@ > > > > > +/* end Shipping pages */ > > > > > + > > > > > + > > > > > +/* l10nstats */ > > > > > +.exhibit-facet-body {height: 5em !important;} > > > > > > > > This doesn't belong in a site-wide stylesheet. > > > > > > > Why? Aren't these rules applicable to a bunch of l10nstats pages? > > > As long as it's commented there's nothing wrong with putting app specific > > > CSS in the "global" one. > > > > The .exhibit-facet-body is automatically generated by all exhibits, and that > > rule was only supposed to apply on that one page. There's no indication that > > this would be a good site-wide setting. > > > Feeling very dim right now. If class="exhibit-facet-body" is used on > multiple pages isn't it good to have it in the global file? Why would it be a good choice to make the facets on all exhibits smaller than default? Technically, the rule is also regressing a feature in exhibit facets, if you compare https://l10n-stage-sj.mozilla.org/shipping/dashboard?locale=da and https://l10n-stage-sj.mozilla.org/shipping/milestones, you'll see that on the dashboard with the reduced height, manually adjusting the height with the mouse is broken. Leave that rule in the template, and we'll fix that in a follow up? > > > > @@ +135,5 @@ > > > > > +.exhibit-facet-body {height: 5em !important;} > > > > > +div.thumbnail {border: solid black 1px; margin: 2px;} > > > > > +.success {background-color: green;} > > > > > +.warning {background-color: orange;} > > > > > +.failure {background-color: red;} > > > > > > > > These conflict with waterfall.css. > > > > > > > Do they? I can't see that. > > > > waterfall.css says: > > +.success {background-color: rgba(0, 128, 0, 0.5);} > > > > style.css says: (as does tbpl.css) > > +.success {background-color: green;} > > > So, waterfall.css overrides with a different colour? What's wrong with that? > Semantically I don't see what's wrong with it but from a user point of view > if the colour for "success" is odd on one page. .success and friends aren't defined in a site-wite consistent way. Unifying that shouldn't be part of this bug, and thus the rules shouldn't be in a site-wide stylesheet at this point.
Attachment #542482 - Flags: review?(l10n) → review-
Also, as discussed no more l10nstats stuff or pushes1 stuff in the global style.css. This prevents any potential confusion with all the various .(success|warning|etc) since there is no global on in style.css anymore.
Attachment #542482 - Attachment is obsolete: true
Attachment #543799 - Flags: review?(l10n)
Comment on attachment 543799 [details] [diff] [review] Corrections to all l10nstats inline css -> external r=me. thanks for the patience and perseverance. potential follow up nit: if the tree progress histograms look funky, remove the black border for .bar in tree_progress.css.
Attachment #543799 - Flags: review?(l10n) → review+
That was quick :) https://github.com/mozilla/elmo/commit/99642a8038e18166c506471c9bdc11a89c9bc2f7
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: 670704
(In reply to comment #35) > potential follow up nit: if the tree progress histograms look funky, remove > the black border for .bar in tree_progress.css. Filed bug 670704
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: