Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Extract About dialog from main HTML file so that we can override it easily in Edge Code #2329

Merged
merged 3 commits into from Dec 12, 2012

Conversation

Projects
None yet
2 participants
Member

njx commented Dec 11, 2012

The only difference in main-view.html between Brackets and Edge Code is the about dialog. Since we want to move to having the differences in Edge Code be an overlay instead of a full fork, we want to localize differences as much as possible into separate minimally-sized files, so that as much is shared as possible.

Note that while I only moved the About dialog into a separate file, it would make sense to take some of the other one-off dialogs that are only used by one module and break them out similarly; there's really no reason for dialog IDs/templates for these one-off dialogs to be centralized in Dialogs.js. I was tempted to just go ahead and do that, but for some dialogs it might additionally make sense to combine them into a single dialog template a la #258.

The dialogs that seem to be true one-offs are update-dialog and project-settings-dialog; all the other dialogs seem similar enough that they probably ought to be combined somehow. If you think it's worthwhile to go ahead and break update-dialog and project-settings-dialog out as part of this pull request, let me know and I can do it easily.

src/help/HelpCommandHandlers.js
@@ -55,11 +56,17 @@ define(function (require, exports, module) {
}
function _handleAboutDialog() {
+ var templateVars = $.extend({
+ ABOUT_ICON : brackets.config.about_icon,
+ APP_NAME_ABOUT_BOX : brackets.config.app_name_about
@njx

njx Dec 11, 2012

Member

Note that VERSION no longer seems to be used in any of the dialogs directly (it's only used within other strings, and gets substituted into them in src/strings.js). So I took it out here.

@ghost ghost assigned jasonsanjose Dec 11, 2012

src/help/HelpCommandHandlers.js
+ APP_NAME_ABOUT_BOX : brackets.config.app_name_about
+ }, Strings);
+
+ var $template = Mustache.render(AboutDialogTemplate, templateVars);
@jasonsanjose

jasonsanjose Dec 11, 2012

Member

I think this should be just template since this isn't a jQuery object.

@njx

njx Dec 11, 2012

Member

Weird--I meant to wrap the render in $(). I actually have no idea why this code works without it!

src/htmlContent/about-dialog.html
@@ -0,0 +1,18 @@
+<div class="about-dialog template modal hide">
@jasonsanjose

jasonsanjose Dec 11, 2012

Member

Are the template and hide classes still required now that this isn't baked into main-view.html?

@njx

njx Dec 11, 2012

Member

Nope. Removed. Also moved the removal of the template class from the clone into showModalDialog() since it shouldn't apply if you're passing an actual template.

Member

jasonsanjose commented Dec 11, 2012

Seems appropriate to do update-dialog and project-settings-dialog while your at it.

Initial review complete.

Member

njx commented Dec 11, 2012

Updates pushed, ready for re-review.

src/help/HelpCommandHandlers.js
if (buildInfo) {
- $("#about-build-number").text(" (" + buildInfo + ")");
+ $("#about-build-number", $template).text(" (" + buildInfo + ")");
@jasonsanjose

jasonsanjose Dec 12, 2012

Member

I just realized we don't have to do this programmatically any more. Before this change, the template had to render early and couldn't wait for the buildInfo to resolve. Now that we build the template just-in-time, we can update the template to use a BUILD_INFO template var.

Member

jasonsanjose commented Dec 12, 2012

Looks good. But one more comment to clean up some programmatic text injection.

Member

njx commented Dec 12, 2012

Done. Ready for another round.

Member

jasonsanjose commented Dec 12, 2012

Merging.

jasonsanjose added a commit that referenced this pull request Dec 12, 2012

Merge pull request #2329 from adobe/nj/extract-about-dialog
Extract About dialog from main HTML file so that we can override it easily in Edge Code

@jasonsanjose jasonsanjose merged commit cc7e4c9 into master Dec 12, 2012

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