Refactor snippet editor and save snippet icon url. #5

Merged
merged 1 commit into from Oct 31, 2011

Conversation

Projects
None yet
2 participants
Owner

Osmose commented Oct 25, 2011

Switches to using ICanHaz for JS templating and reorganizes the
snippet editor to be a bit more manageable.

In addition, the snippet editor saves the icon URL in a comment
in the snippet so the url box keeps the icon URL between saves.

+ <div class="snippet">
+ [[#icon]]
+ <!--icon:[[ url ]]-->
+ <img class="icon" src="[[ data ]]" />
@tofumatt

tofumatt Oct 25, 2011

Member

If this is HTML5 you don't need the trailing slash for img tags. Also: no alt attribute?

@Osmose

Osmose Oct 25, 2011

Owner

Snippets unfortunately aren't HTML5, but have to be valid XML: http://dxr.mozilla.org/mozilla/mozilla-central/browser/base/content/aboutHome.js.html#l239

As for the alt attribute, would an empty alt do? If not, could we use an empty alt and add a bug for letting the user specify the alt text? (This also brings up issues of localizing the alt text when the snippet is submitted for translation)

@tofumatt

tofumatt Oct 27, 2011

Member

Empty alt is bad; it's a legit thing for screenreaders and such to use. If this is on the about:home page I'd argue we put our best foot forward and make it sexy, accessible HTML.

How hard would it be to have an alt attribute in here?

+register = template.Library()
+
+
+SYMBOLS = [
@tofumatt

tofumatt Oct 25, 2011

Member

Hahaha this is awesomely brain-hurty to read at first. I was searching for ASCII art.

+
+@register.tag
+def icanhaz(parser, token):
+ """
@tofumatt

tofumatt Oct 25, 2011

Member

Docstrings first sentence should be one line and on the first line; anything else comes after an empty line.

apps/homesnippets/templatetags/icanhaz.py
+
+
+class ICanHazNode(template.Node):
+ """Performs a find and replace on the contents of the node."""
@tofumatt

tofumatt Oct 25, 2011

Member

Can you "perform a find (and replace)"? Isn't this a basic parser? Why not just call it that?

apps/homesnippets/templatetags/icanhaz.py
+ def __init__(self, nodelist):
+ self.nodelist = nodelist
+
+ def render(self, context, ):
@tofumatt

tofumatt Oct 25, 2011

Member

Extra comma/space?

apps/homesnippets/tests/test_templatetags.py
+
+ def test_basic(self):
+ """
+ icanhaz replaces double and triple square brackets with curly brackets.
@tofumatt

tofumatt Oct 25, 2011

Member

Should be on the line above. (See earlier note about docstrings.)

@tofumatt

tofumatt Oct 25, 2011

Member

Also, docstrings should be a complete sentence. I don't feel like this informs the developer of the test's how/why.

site_media/snippetBodyWidget.js
- }
+ function updatePreview() {
+ var code = ich.snippet_template(snippet, true);
+ elems['code'].val(code);
@tofumatt

tofumatt Oct 25, 2011

Member

Dot syntax is shorter for what it's worth, and also nicer and more natural, as you're really accessing an object property. elems.code not elems['code'].

site_media/snippetBodyWidget.js
+ url: '/base64encode?url=' + encodeURIComponent(icon_url),
+ dataType: 'json',
+ error: function() {
+ alert('Error encoding icon. Please check that the icon URL points to a valid PNG image.');
@tofumatt

tofumatt Oct 25, 2011

Member

alert() for user notifications? Kinda annoying. Maybe at least TODO: make pretty?

@Osmose

Osmose Oct 25, 2011

Owner

FWIW, this is only used in the django admin interface, so it's not user-facing. But yeah, it's ugly.

@tofumatt

tofumatt Oct 25, 2011

Member

Admins are users too, but if you aren't expecting this to happen a lot I suppose it's alright.

site_media/snippetBodyWidget.js
- }
+ // Bind events and do UI
+ $('#snippet-text').bind('change keyup', function() {
+ snippet['text'] = wiki2link(elems['text'].val());
@tofumatt

tofumatt Oct 25, 2011

Member

You can use dot notation in here too.

site_media/snippetBodyWidget.js
+ (function() {
+ var snippet_code = elems['code'].val();
+ if (snippet_code.match(/<!--basic-->/) === null) {
+ if (snippet_code != '') {
@tofumatt

tofumatt Oct 25, 2011

Member

!== would be preferred in JS, yo.

site_media/snippetBodyWidget.js
+ url: (icon_matches ? icon_matches[1] : ''),
+ data: (img_matches ? img_matches[1] : '')
+ };
+ elems['icon_url'].val(snippet['icon']['url']);
@tofumatt

tofumatt Oct 25, 2011

Member

More dot syntax opportunities.

site_media/snippetBodyWidget.js
+ // Be dumb and throw a regex at it
@tofumatt

tofumatt Oct 25, 2011

Member

Kind of a weird comment o_O

Member

tofumatt commented Oct 25, 2011

I'm pretty OK with this; lots of nits, but it's overall good. Could you fix them up and I'll have another quick run-through?

apps/homesnippets/tests/test_templatetags.py
+
+class TestICanHaz(TestCase):
+
+ def setUp(self):
@tofumatt

tofumatt Oct 27, 2011

Member

I'm probably a bit overzealous about docs/docstrings, but I really feel like every Class/method ought have one. Even if it's simply: "why this method exists".

I feel like it makes our code look really well-maintained and it means there's less groking needed when someone new checks out your code.

+from nose.tools import eq_
+
+class TestICanHaz(TestCase):
+
@tofumatt

tofumatt Oct 27, 2011

Member

What I wrote below applies to the class as well. What is this testing? Even one line (even if it's a bit repetitive based on the Class's name) will do.

apps/homesnippets/tests/test_templatetags.py
+ template = get_template_from_string(template_str)
+ result = template.render(self.context).strip()
+
+ # Single brackets should be preserved, but double and triple
@tofumatt

tofumatt Oct 27, 2011

Member

Make this the third argument to your eq_() call and it will show up if the test fails. That's more useful.

+ var Renderer = function() {};
+
+ Renderer.prototype = {
+ otag: "{{",
@tofumatt

tofumatt Oct 27, 2011

Member

I like that your quotes are consistent, but make them consistently single-quotes. Good rule of thumb is "construct your string literals the same as you would in Python" (re: when to use single/double quotes).

@tofumatt

tofumatt Oct 27, 2011

Member

Wait a second -- I'm a moron. This is a library. It's not your fault.

lololololololololol

Member

tofumatt commented Oct 27, 2011

Let me know what you think about the alt attribute. Otherwise I think this is good r+wc.

Owner

Osmose commented Oct 31, 2011

Fixing the alt attribute is going to be a bit more work and is out of the scope of this pull request, I think.

I'll leave out the alt attribute for now; I've filed a bug for using a semantic alternative to an image tag: https://bugzilla.mozilla.org/show_bug.cgi?id=698596

Refactor snippet editor and save snippet icon url.
Switches to using ICanHaz for JS templating and reorganizes the
snippet editor to be a bit more manageable.

In addition, the snippet editor saves the icon URL in a comment
in the snippet so the url box keeps the icon URL between saves.
Member

tofumatt commented Oct 31, 2011

That works for me. ARRRRRRRRR PLUS!

@Osmose Osmose merged commit 83c230e into mozilla:master Oct 31, 2011

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