Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Combine jdiel's "language switching" pull request with tvoliter's templatization of index.html using mustache #1358

Merged
merged 20 commits into from Aug 14, 2012

Conversation

tvoliter
Copy link
Contributor

  • includes jdiel's "language switching" pull request Added Language Switching #1285 which includes DE translations, a menu and UI for switching languages, and string lookup support through i18n.js
  • added new module htmlContentLoad.js that dynamically loads main-view.html, localizes it using i18n.js + mustache.js, and injects it into the body of index.html
  • body of index.html moved to main-view.html
  • modified SideBarView.js to depend on htmlContentLoad since this module modifies HTML on load.

3rd Party Code Notice: Mustache.js uses an MIT license: https://github.com/janl/mustache.js/blob/master/LICENSE. I will need to complete the Brackets third party check list before this pull request can be merged.

…ly one loc file!

- added file htmlContentLoad.js that is a require module that dynamically renders html templates and localized strings in the body of teh page
- break apart index.html into main-view.html, modal-windows.html, and side-bar.html
@ghost ghost assigned jasonsanjose Aug 10, 2012
@@ -0,0 +1,176 @@
<!--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs copyright header

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting a jslint error for this html template fragment. I'm not sure what the solution is though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this too. I think JSLint expects it to be a fully formed html structure since it sees the .html suffix. Since this is really an html fragment it doesn’t have or tags.

I don’t know if the JSLint options you can put in comments work in HTML comments.

@jasonsanjose
Copy link
Member

Initial review complete.

@jasonsanjose
Copy link
Member

Also, we need to verify the 3rd party code checklist.

@jasonsanjose
Copy link
Member

Need to merge with master

@@ -22,144 +22,17 @@
*/

/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global define */
/*global $, window, define */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

window is unused. remove this jslint config option.

@jasonsanjose
Copy link
Member

The working set wasn't working properly when I did some testing. See my comment in SidebarView.js.

<ul data-dropdown="dropdown"></ul>
</div>
<!-- HTML content is dynamically loaded and rendered by the htmlContentLoad
module. Any modules that depend on or modify HTML during load should require the htmlContentLoad module so that it is loaded first. -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment line should be split so that it fits in 80 columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I also updated the comment now that I am using an event to signal the dom is ready.

// The htmlContentLoad module renders all of html in Bracktes.
// If a module depends on the DOM being fulling formed before it is loaded
// then the module should listen for the event "htmlContentLoadComplete"
require("htmlContent/htmlContentLoad");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have seen this sooner, but do we need a completely separate module anymore? htmlContentLoad.js won't be reused anywhere else. It seems like we could inline the Mustache.render() call inside _onReady() in brackets.js.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can make this change, then $(brackets).trigger("htmlContentLoadComplete"); would immediately follow Mustache.render(). I think that would be a cleaner approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I removed that module and moved the functionality to brackets.js

Now SidebarView.js and ProjectManager.js listen for the htmlContentLoadComplete event since they both init jquery vars on load.

From: Jason San Jose [mailto:notifications@github.com]
Sent: Tuesday, August 14, 2012 10:33 AM
To: adobe/brackets
Cc: Ty Voliter
Subject: Re: [brackets] Combine jdiel's "language switching" pull request with tvoliter's templatization of index.html using mustache (#1358)

In src/brackets.js:

@@ -50,6 +57,12 @@ define(function (require, exports, module) {

 require("thirdparty/path-utils/path-utils.min");

 require("thirdparty/smart-auto-complete/jquery.smart_autocomplete");
  • // Load HTML Content
  • // The htmlContentLoad module renders all of html in Bracktes.
  • // If a module depends on the DOM being fulling formed before it is loaded
  • // then the module should listen for the event "htmlContentLoadComplete"
  • require("htmlContent/htmlContentLoad");

Should have seen this sooner, but do we need a completely separate module anymore? htmlContentLoad.js won't be reused anywhere else. It seems like we could inline the Mustache.render() call inside _onReady() in brackets.js.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1358/files#r1374771.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

From: Jason San Jose [mailto:notifications@github.com]
Sent: Tuesday, August 14, 2012 10:34 AM
To: adobe/brackets
Cc: Ty Voliter
Subject: Re: [brackets] Combine jdiel's "language switching" pull request with tvoliter's templatization of index.html using mustache (#1358)

In src/brackets.js:

@@ -50,6 +57,12 @@ define(function (require, exports, module) {

 require("thirdparty/path-utils/path-utils.min");

 require("thirdparty/smart-auto-complete/jquery.smart_autocomplete");
  • // Load HTML Content
  • // The htmlContentLoad module renders all of html in Bracktes.
  • // If a module depends on the DOM being fulling formed before it is loaded
  • // then the module should listen for the event "htmlContentLoadComplete"
  • require("htmlContent/htmlContentLoad");

If we can make this change, then $(brackets).trigger("htmlContentLoadComplete"); would immediately follow Mustache.render(). I think that would be a cleaner approach.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1358/files#r1374784.

redmunds and others added 2 commits August 14, 2012 10:53
- modified SidebarView.js and ProjectManager.js to init DOM dependant variables after htmlContentLoadComplete event
- modified SidebarView.js and ProjectManager.js to init DOM dependant variables after htmlContentLoadComplete event
@tvoliter
Copy link
Contributor Author

Working on this now…

From: Jason San Jose [mailto:notifications@github.com]
Sent: Monday, August 13, 2012 3:35 PM
To: adobe/brackets
Cc: Ty Voliter
Subject: Re: [brackets] Combine jdiel's "language switching" pull request with tvoliter's templatization of index.html using mustache (#1358)

Need to merge with master


Reply to this email directly or view it on GitHubhttps://github.com//pull/1358#issuecomment-7710502.

…to tvoliter/localizationSupport

* origin/tvoliter/localizationSupport:
  - removed htmlContentLoad.js module. Functionality moved to brackets.js - modified SidebarView.js and ProjectManager.js to init DOM dependant variables after htmlContentLoadComplete event
exports.CMD_SHOW_PERF_DATA = "Show Performance Data";
exports.CMD_NEW_BRACKETS_WINDOW = "New Brackets Window";
exports.CMD_USE_TAB_CHARS = "Use Tab Characters";
'use strict';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be double quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

From: Glenn Ruehle [mailto:notifications@github.com]
Sent: Tuesday, August 14, 2012 1:10 PM
To: adobe/brackets
Cc: Ty Voliter
Subject: Re: [brackets] Combine jdiel's "language switching" pull request with tvoliter's templatization of index.html using mustache (#1358)

In src/strings.js:

  • exports.CMD_TOGGLE_QUICK_EDIT = "Quick Edit";
  • exports.CMD_QUICK_EDIT_PREV_MATCH = "Previous Match";
  • exports.CMD_QUICK_EDIT_NEXT_MATCH = "Next Match";
  • exports.CMD_NEXT_DOC = "Next Document";
  • exports.CMD_PREV_DOC = "Previous Document";

  • // Debug menu commands
  • exports.DEBUG_MENU = "Debug";
  • exports.CMD_REFRESH_WINDOW = "Reload Brackets";
  • exports.CMD_SHOW_DEV_TOOLS = "Show Developer Tools";
  • exports.CMD_RUN_UNIT_TESTS = "Run Tests";
  • exports.CMD_JSLINT = "Enable JSLint";
  • exports.CMD_SHOW_PERF_DATA = "Show Performance Data";
  • exports.CMD_NEW_BRACKETS_WINDOW = "New Brackets Window";
  • exports.CMD_USE_TAB_CHARS = "Use Tab Characters";
  • 'use strict';

Should be double quotes.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1358/files#r1376639.

…onSupport

* origin/master: (33 commits)
  Update About dialog sprint number.
  Update README.md
  don't cache itemsPerPage
  change height to see 8 items
  code review changes
  simplify menu/command lookup
  handle same command in multiple menus
  support for scrolling code hint menu
  Respond to code review: add tag hinting sanity checks; add filtered tag hinting test; fix typo.
  Set shouldShowHintsOnKeyUp to false as added precaution as we pass key events to hintList.
  Check keys on keyPress events and invoke code hints on keyUp events.
  Unit tests for HTML attribute code hinting, plus a few couple extra tests for tag code hinting.
  Added "brackets" global variable, removed space after exclamation mark (JSLint)
  implement abortQuit callback
  add comment for brackets-shell migration
  Revert verification change in unit test
  Revert "restore unit test verification step"
  Revert "restore unit test verification step". Detect brackets-shell vs. brackets-app.
  Fix HTMLUtils unit tests by adding the new parameter for the new property. Also make it consistent by setting the new property to be true regardless of the value is empty or not as long as there is a equal sign.
  restore unit test verification step
  ...

Conflicts:
	src/index.html
	src/strings.js
…onSupport

* origin/master: (33 commits)
  Update About dialog sprint number.
  Update README.md
  don't cache itemsPerPage
  change height to see 8 items
  code review changes
  simplify menu/command lookup
  handle same command in multiple menus
  support for scrolling code hint menu
  Respond to code review: add tag hinting sanity checks; add filtered tag hinting test; fix typo.
  Set shouldShowHintsOnKeyUp to false as added precaution as we pass key events to hintList.
  Check keys on keyPress events and invoke code hints on keyUp events.
  Unit tests for HTML attribute code hinting, plus a few couple extra tests for tag code hinting.
  Added "brackets" global variable, removed space after exclamation mark (JSLint)
  implement abortQuit callback
  add comment for brackets-shell migration
  Revert verification change in unit test
  Revert "restore unit test verification step"
  Revert "restore unit test verification step". Detect brackets-shell vs. brackets-app.
  Fix HTMLUtils unit tests by adding the new parameter for the new property. Also make it consistent by setting the new property to be true regardless of the value is empty or not as long as there is a equal sign.
  restore unit test verification step
  ...

Conflicts:
	src/index.html
	src/strings.js
@tvoliter
Copy link
Contributor Author

Code code review fixes pushed and I merged with master

@@ -22,145 +22,17 @@
*/

/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global define */
/*global $, define */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jquery $ is unused

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

From: Jason San Jose [mailto:notifications@github.com]
Sent: Tuesday, August 14, 2012 1:55 PM
To: adobe/brackets
Cc: Ty Voliter
Subject: Re: [brackets] Combine jdiel's "language switching" pull request with tvoliter's templatization of index.html using mustache (#1358)

In src/strings.js:

@@ -22,145 +22,17 @@

*/

/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */

-/*global define */

+/*global $, define */

jquery $ is unused


Reply to this email directly or view it on GitHubhttps://github.com//pull/1358/files#r1377055.

$submit.attr("disabled", false);
}

var $modal = $("<div class='modal hide' />");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll file a general issue to move blocks such as this one into a template.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants