-
Notifications
You must be signed in to change notification settings - Fork 7.5k
[Initial review] Create new rule from CSS quick edit #5532
Changes from 27 commits
8a283d4
aaac429
9c1c60f
8e66aa7
3f580cd
c89ed3c
af1ea12
b61fa58
b9dd8b8
21fc0bd
90c2f55
e0c6789
a719c78
a3a7197
6e632cf
39fe9ed
81c3bee
721c5e1
8dfbf0f
54909b0
fba79f4
dfbc99f
a8ed43f
fdcd299
1d730b5
b564fef
4bbc37d
340ae12
cfc8f2a
6e3f12d
9a6423d
1d5ee47
c64412b
74ed9b8
167e419
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,16 +23,25 @@ | |
|
|
||
|
|
||
| /*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */ | ||
| /*global define, $, CodeMirror, window */ | ||
| /*global define, $, CodeMirror, window, Mustache */ | ||
|
|
||
| define(function (require, exports, module) { | ||
| "use strict"; | ||
|
|
||
| // Load dependent modules | ||
| var CSSUtils = require("language/CSSUtils"), | ||
| DocumentManager = require("document/DocumentManager"), | ||
| DropdownEventHandler = require("utils/DropdownEventHandler").DropdownEventHandler, | ||
| EditorManager = require("editor/EditorManager"), | ||
| Editor = require("editor/Editor").Editor, | ||
| FileIndexManager = require("project/FileIndexManager"), | ||
| HTMLUtils = require("language/HTMLUtils"), | ||
| MultiRangeInlineEditor = require("editor/MultiRangeInlineEditor").MultiRangeInlineEditor; | ||
| Menus = require("command/Menus"), | ||
| MultiRangeInlineEditor = require("editor/MultiRangeInlineEditor").MultiRangeInlineEditor, | ||
| PopUpManager = require("widgets/PopUpManager"), | ||
| Strings = require("strings"); | ||
|
|
||
| var StylesheetsMenuTemplate = require("text!htmlContent/stylesheets-menu.html"); | ||
|
|
||
| /** | ||
| * Given a position in an HTML editor, returns the relevant selector for the attribute/tag | ||
|
|
@@ -78,6 +87,33 @@ define(function (require, exports, module) { | |
| return selectorName; | ||
| } | ||
|
|
||
| /** | ||
| * @private | ||
| * Create the list of stylesheets in the dropdown menu. | ||
| * @return {string} The html content | ||
| */ | ||
| function _renderList(cssFileInfos) { | ||
| var templateVars = { | ||
| styleSheetList : cssFileInfos | ||
| }; | ||
|
|
||
| return Mustache.render(StylesheetsMenuTemplate, templateVars); | ||
| } | ||
|
|
||
| /** | ||
| * @private | ||
| * Add a new rule for the given selector to the given document, then add the rule to the | ||
| * given inline editor. | ||
| * @param {string} selectorName The selector to create a rule for. | ||
| * @param {MultiRangeInlineEditor} inlineEditor The inline editor to display the new rule in. | ||
| * @param {Document} styleDoc The document the rule should be inserted in. | ||
| */ | ||
| function _addRule(selectorName, inlineEditor, styleDoc) { | ||
| var newRuleInfo = CSSUtils.addRuleToDocument(styleDoc, selectorName, Editor.getUseTabChar(), Editor.getSpaceUnits()); | ||
| inlineEditor.addAndSelectRange(selectorName, styleDoc, newRuleInfo.range.from.line, newRuleInfo.range.to.line); | ||
| inlineEditor.editor.setCursorPos(newRuleInfo.pos.line, newRuleInfo.pos.ch); | ||
| } | ||
|
|
||
| /** | ||
| * This function is registered with EditManager as an inline editor provider. It creates a CSSInlineEditor | ||
| * when cursor is on an HTML tag name, class attribute, or id attribute, find associated | ||
|
|
@@ -89,6 +125,7 @@ define(function (require, exports, module) { | |
| * or null if we're not going to provide anything. | ||
| */ | ||
| function htmlToCSSProvider(hostEditor, pos) { | ||
|
|
||
| // Only provide a CSS editor when cursor is in HTML content | ||
| if (hostEditor.getLanguageForSelection().getId() !== "html") { | ||
| return null; | ||
|
|
@@ -107,19 +144,135 @@ define(function (require, exports, module) { | |
| return null; | ||
| } | ||
|
|
||
| var result = new $.Deferred(); | ||
| var result = new $.Deferred(), | ||
| cssInlineEditor, | ||
| cssFileInfos = [], | ||
| $newRuleButton, | ||
| $dropdown, | ||
| $dropdownItem, | ||
| dropdownEventHandler; | ||
|
|
||
| /** | ||
| * @private | ||
| * Close the dropdown. | ||
| */ | ||
| function _closeDropdown() { | ||
| // Since we passed "true" for autoRemove to addPopUp(), this will | ||
| // automatically remove the dropdown from the DOM. Also, PopUpManager | ||
| // will call _cleanupDropdown(). | ||
| if ($dropdown) { | ||
| PopUpManager.removePopUp($dropdown); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @private | ||
| * Remove the various event handlers that close the dropdown. This is called by the | ||
| * PopUpManager when the dropdown is closed. | ||
| */ | ||
| function _cleanupDropdown() { | ||
| $("html").off("click", _closeDropdown); | ||
| dropdownEventHandler = null; | ||
| $dropdown = null; | ||
|
|
||
| EditorManager.focusEditor(); | ||
| } | ||
|
|
||
| function _onSelect($link) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the other functions here have jsdoc comments. I'm not actually very particular about jsdoc comments for simple interior functions like this, but it would be more consistent to have a comment since the others do.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @redmunds - do you want to add comments for those functions? If not I can take a stab at it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add jsdoc comments |
||
| var path = $link.data("path"); | ||
|
|
||
| if (path) { | ||
| DocumentManager.getDocumentForPath(path).done(function (styleDoc) { | ||
| _addRule(selectorName, cssInlineEditor, styleDoc); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @private | ||
| * Show or hide the stylesheets dropdown. | ||
| */ | ||
| function _showDropdown() { | ||
| // If the dropdown is already visible, just return (so the root click handler on html | ||
| // will close it). | ||
| if ($dropdown) { | ||
| return; | ||
| } | ||
|
|
||
| Menus.closeAll(); | ||
|
|
||
| $dropdown = $(_renderList(cssFileInfos)); | ||
|
|
||
| var toggleOffset = $newRuleButton.offset(); | ||
| $dropdown | ||
| .css({ | ||
| left: toggleOffset.left, | ||
| top: toggleOffset.top + $newRuleButton.outerHeight() | ||
| }) | ||
| .appendTo($("body")); | ||
|
|
||
| $("html").on("click", _closeDropdown); | ||
|
|
||
| dropdownEventHandler = new DropdownEventHandler($dropdown, _onSelect, _cleanupDropdown); | ||
| dropdownEventHandler.open(); | ||
|
|
||
| $dropdown.focus(); | ||
| } | ||
|
|
||
| /** | ||
| * @private | ||
| * Display list of stylesheets in project, then create a new rule in the given stylesheet and | ||
| * add it to the given inline editor. | ||
| * @param {string} selectorName The selector to create a rule for. | ||
| * @param {MultiRangeInlineEditor} inlineEditor The inline editor to display the new rule in. | ||
| */ | ||
| function _handleNewRule() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This appears to be left over from some kind of refactoring. The doc comments are out of date, and this function just calls
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Also made button toggle dropdown. |
||
| _showDropdown(); | ||
| } | ||
|
|
||
| /** | ||
| * @private | ||
| * Checks to see if there are any stylesheets in the project, and returns the appropriate | ||
| * "no rules"/"no stylesheets" message accordingly. | ||
| * @return {$.Promise} a promise that is resolved with the message to show. Never rejected. | ||
| */ | ||
| function _getNoRulesMsg() { | ||
| var result = new $.Deferred(); | ||
| FileIndexManager.getFileInfoList("css").done(function (fileInfos) { | ||
| result.resolve(fileInfos.length ? Strings.CSS_QUICK_EDIT_NO_MATCHES : Strings.CSS_QUICK_EDIT_NO_STYLESHEETS); | ||
| }); | ||
| return result; | ||
| } | ||
|
|
||
| CSSUtils.findMatchingRules(selectorName, hostEditor.document) | ||
| .done(function (rules) { | ||
| if (rules && rules.length > 0) { | ||
| var cssInlineEditor = new MultiRangeInlineEditor(rules); | ||
| cssInlineEditor.load(hostEditor); | ||
|
|
||
| result.resolve(cssInlineEditor); | ||
| } else { | ||
| // No matching rules were found. | ||
| result.reject(); | ||
| } | ||
| cssInlineEditor = new MultiRangeInlineEditor(rules || [], _getNoRulesMsg); | ||
| cssInlineEditor.load(hostEditor); | ||
|
|
||
| var $header = $(".inline-editor-header", cssInlineEditor.$htmlContent); | ||
| $newRuleButton = $("<button class='stylesheet-button btn btn-mini disabled'/>") | ||
| .text(Strings.BUTTON_NEW_RULE) | ||
| .on("click", function (e) { | ||
| if (!$newRuleButton.hasClass("disabled")) { | ||
| _handleNewRule(); | ||
| } | ||
| e.stopPropagation(); | ||
| }); | ||
| $header.append($newRuleButton); | ||
|
|
||
| result.resolve(cssInlineEditor); | ||
|
|
||
| // Now that dialog has been built, collect list of stylesheets | ||
| FileIndexManager.getFileInfoList("css") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it seems like the calls to
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My original thinking was that when we go to show the "no rules" message, if the user creates a stylesheet in between the time the inline editor was opened and the time we show the message, we'd want to know that and show the other message. However, we'd have to do other work to detect that and refresh the message/re-enable the button anyhow (which we're not doing currently). I think we should do that, but it's a separate task.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, I see. Yes, that can be a separate task. |
||
| .done(function (fileInfos) { | ||
| cssFileInfos = fileInfos; | ||
|
|
||
| // "New Rule" button is disabled by default and gets enabled | ||
| // here if there are any stylesheets in project | ||
| if (cssFileInfos.length > 0) { | ||
| $newRuleButton.removeClass("disabled"); | ||
| } | ||
| }); | ||
| }) | ||
| .fail(function () { | ||
| console.log("Error in findMatchingRules()"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit leaky to me. The
addPopUpcall was made inDropdownEventHandler. It seems to me that perhapsDropdownEventHandler. This is the only call toPopUpManagerin this file, which leads me to think there should really be a method on theDropdownEventHandlerto do this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I introduced a
DropdownEventHandler.close()method.