Use templates instead of jQuery in the Debug Commands extension #3336

Merged
merged 4 commits into from May 13, 2013

Projects

None yet

4 participants

@TomMalbran
Contributor

I moved the jQuery HTML creation to a Mustache template for both the performance data and the language switcher, making the code look a lot cleaner.

@jasonsanjose
Member

Tagging open for Sprint 25. Please don't merge in sprint 24.

@jbalsas jbalsas was assigned May 2, 2013
@jbalsas
Member
jbalsas commented May 2, 2013

Reviewing...

@jbalsas jbalsas and 1 other commented on an outdated diff May 2, 2013
src/nls/root/strings.js
@@ -331,6 +324,13 @@ define({
"UNKNOWN_ERROR" : "Unknown internal error.",
// For NOT_FOUND_ERR, see generic strings above
+
+ // extensions/default/DebugComments - Switch language
+ "LANGUAGE_TITLE" : "Switch Language",
+ "LANGUAGE_MESSAGE" : "Language:",
+ "LANGUAGE_SUBMIT" : "Reload {APP_NAME}",
+ "LANGUAGE_SYSTEM_DEFAULT" : "System Default",
+
@jbalsas
jbalsas May 2, 2013 Member

This changes are currently in conflict. Since we'll be working on #3575, we could just discard this here and take care of it all together.

@TomMalbran
TomMalbran May 2, 2013 Contributor

I'll remove this, I did this change before the other request where I found that there were a lot of this problems of the strings not being where they should be.

@jbalsas jbalsas commented on the diff May 2, 2013
...ns/default/DebugCommands/htmlContent/perf-dialog.html
@@ -0,0 +1,23 @@
+<div class="modal">
+ <div class="modal-header">
+ <a href="#" class="close">&times;</a>
+ <h1 class="dialog-title">Performance Data</h1>
+ <div style="text-align: right">
+ Raw data (copy paste out):
@jbalsas
jbalsas May 2, 2013 Member

These strings could be localized. This is a very specific dialog though, so not sure if it's really worth it, or if it's even here to stay. @jasonsanjose what do you think?

@jbalsas
jbalsas May 2, 2013 Member

@TomMalbran you heard the man ;)

@jbalsas jbalsas commented on an outdated diff May 2, 2013
...ns/default/DebugCommands/htmlContent/perf-dialog.html
+<div class="modal">
+ <div class="modal-header">
+ <a href="#" class="close">&times;</a>
+ <h1 class="dialog-title">Performance Data</h1>
+ <div style="text-align: right">
+ Raw data (copy paste out):
+ <textarea rows="1" style="width:30px; height:8px; overflow: hidden; resize: none" id="brackets-perf-raw-data">{{delimitedPerfData}}</textarea>
+ </div>
+ </div>
+ <div class="modal-body" style="padding: 0; max-height: 500px; overflow: auto; width: 592px">
+ <table class="zebra-striped condensed-table">
+ <thead><th>Operation</th><th>Time (ms)</th></thead>
+ <tbody>
+ {{#perfData}}
+ <tr>
+ <td>{{testName}}</td>
@jbalsas
jbalsas May 2, 2013 Member

Could you try wrapping this in a div and setting a width here so that the second column (the one with the results) doesn't get pushed to the right for long paths?

I'm currently in the case where I don't see them right ahead and have to scroll to get them to show.

@jbalsas jbalsas commented on the diff May 2, 2013
src/extensions/default/DebugCommands/main.js
}
}
});
- $select.val(curLocale);
+ var template = Mustache.render(LanguageDialogTemplate, $.extend({languages: languages}, Strings));
@jbalsas
jbalsas May 2, 2013 Member

It seems a little bit overkill to pass the full Strings object here since we're just using 4 of them. Also, if we use $.extend, I think Strings should go in the first place to avoid extra operations (not that it should be noticeable, but better to iterate through less keys).

I really like that with this approach, the strings in the template are called like the variables in the string files. It makes it very easy to find them later.

@njx
njx May 2, 2013 Member

The problem with Strings being in the first place is that it will actually modify the Strings object, which we don't want.

We've used this pattern elsewhere (and I had the same concern initially, but it doesn't seem to be that much of a performance impact). If we're concerned about performance, the right fix is probably to have a small convenience function that copies over a specified subset of keys.

@jbalsas
jbalsas May 2, 2013 Member

Oh, you're right! I knew we'll be modifying the Strings object but didn't realize that we'll be modifying THE Strings object!

@TomMalbran
TomMalbran May 2, 2013 Contributor

It makes sense that is the object, because to not be it, it would have to copy all the strings, and that isn't a cheap operation, but we are still doing later when using this extend pattern.

Another alternative once it becomes a problem is to split the strings into extensions/dialogs/teplates so that then you could just load the strings you want and not all.

@jbalsas
Member
jbalsas commented May 2, 2013

Review complete.

@TomMalbran Looks great! Code definitely is much cleaner now. Just a couple of comments, and the conflicting strings issue.

@TomMalbran
Contributor

@jbalsas Thanks for the review. Changes pushed. I made the urls breakable, so the table should fit in the dialog now.

@jbalsas
Member
jbalsas commented May 3, 2013

@TomMalbran Changes look good, and the performance data now shows properly, thanks for that! Did you rebase with master? 2af6b67 certainly looks like it...

@njx Is it ok if we merge this with 2af6b67 as is?

@njx
Member
njx commented May 3, 2013

Hmmm, 2af6b67 does look weird. Could you try rebasing onto master and seeing if the huge diffs in there go away?

@jbalsas
Member
jbalsas commented May 3, 2013

Yes, if I rebase or just merge with master I only see the right diffs being applied.

@TomMalbran
Contributor

Yes, I merged it with master and since there were conflict issues I fixed them and then fixed the issues mentioned in the same commit. Since the first commit was almost a month ago, that merge was mostly from the last 2 sprints, and the last one had lots of changes.

This is the diff with master: TomMalbran@adobe:master...tom/debug-templates

@TomMalbran
Contributor

The changes here aren't that many, so I could just remake this request if that would be better?

@jbalsas
Member
jbalsas commented May 4, 2013

@TomMalbran. Same as with the other, this seems ok to me, and the commits appear without any additional diffs when I merge them locally, so we should be able to merge as is, no further work needed.

I'll wait a couple of days to merge, though, since I don't want to take any chances of leaving the repo in a strange state.

@TomMalbran
Contributor

Sure, that is ok. Is not such an important issue, so it can wait a few more days. Doing something like git rebase upstream/master would fix the merge issue leaving only the new changes?

@njx
Member
njx commented May 10, 2013

@TomMalbran Yes, you should be able to just rebase it that way. If you run into conflicts that are hard to resolve, then we can just do a merge, but it would be good to rebase it if you can.

@jbalsas
Member
jbalsas commented May 13, 2013

Hey, @TomMalbran, sorry again for taking so long to get back at this and let some other commits slip in between :S

We have a merge conflict coming from #3704, but it looks like it should be quite straightforward to resolve. Could you look into it and merge with master?

@TomMalbran
Contributor

@jbalsas Just merged with master and fixed the conflict issue.

@jbalsas
Member
jbalsas commented May 13, 2013

Looks good. Merging!

Thanks again for your patience on this one :)

@jbalsas jbalsas merged commit e8b1119 into adobe:master May 13, 2013

1 check passed

default The Travis CI build passed
Details
@TomMalbran TomMalbran deleted the TomMalbran:tom/debug-templates branch May 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment