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

Added Code to HtmlUtils to return info when in closing tag #3419

Merged
merged 5 commits into from
Apr 20, 2013
Merged

Added Code to HtmlUtils to return info when in closing tag #3419

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
391342c
Added code to return tagName when in closing tag
WebsiteDeveloper Apr 11, 2013
a079da6
A few refactorings
WebsiteDeveloper Apr 11, 2013
b507ad0
Fixes after first review
WebsiteDeveloper Apr 13, 2013
f50e376
Fixes after second review
WebsiteDeveloper Apr 16, 2013
53cbe28
Minor fixes after review
WebsiteDeveloper Apr 20, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/editor/CSSInlineEditor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ define(function (require, exports, module) {
var tagInfo = HTMLUtils.getTagInfo(editor, pos),
selectorName = "";

if (tagInfo.position.tokenType === HTMLUtils.TAG_NAME) {
if (tagInfo.position.tokenType === HTMLUtils.TAG_NAME || tagInfo.position.tokenType === HTMLUtils.CLOSING_TAG) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to add a new unit test for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

You still need to add a unit test for Quick Edit in closing tag to test this specific change, probably in InlineEditorProviders-test.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems i've missed that

// Type selector
selectorName = tagInfo.tagName;
} else if (tagInfo.position.tokenType === HTMLUtils.ATTR_NAME ||
Expand Down
28 changes: 19 additions & 9 deletions src/language/HTMLUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ define(function (require, exports, module) {

//constants
var TAG_NAME = "tagName",
CLOSING_TAG = "closingTag",
ATTR_NAME = "attr.name",
ATTR_VALUE = "attr.value";

Expand Down Expand Up @@ -399,10 +400,18 @@ define(function (require, exports, module) {
return createTagInfo();
}

// Check to see if this is the closing of a tag (either the start or end)
if (ctx.token.string === ">" || ctx.token.string === "/>" ||
(ctx.token.string.charAt(0) === "<" && ctx.token.string.charAt(1) === "/")) {
return createTagInfo();
// Check to see if this is the closing of a tag (either the start or end) self closing tag
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also update this comment to " the closing of a start tag or self closing tag."

if (ctx.token.string === ">" || ctx.token.string === "/>") {
return createTagInfo(CLOSING_TAG, offset);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this unless there is a specific use of "CLOSING_TAG" in any feature. Otherwise, you may introduce some unit test failures here. So I suggest that you keep the empty tag info.

}

// Check to see if this is the closing of a tag (either the start or end) closing tag
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I should have caught this in the previous round of review. This comment is no longer correct. We're just handling closing tag here.

if (ctx.token.string.charAt(0) === "<" && ctx.token.string.charAt(1) === "/") {
if (ctx.token.string.indexOf(">") === -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

CodeMirror tokenizer keeps ">" as a separate token. So you don't need to check this at all and you also need to remove the else block.

return createTagInfo(CLOSING_TAG, offset, ctx.token.string.slice(2));
Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to adjust the offset here since you strip off two preceding characters and the caller only see the actual tag name without "</".

This change is causing some HTMLUtils unit tests (CodeHintUtils.js) failing. So you need to fix those.

} else {
return createTagInfo(CLOSING_TAG, offset, ctx.token.string.slice(2, ctx.token.string.indexOf(">") + 1));
}
}

// Make sure the cursor is not after an equal sign or a quote before we report the context as a tag.
Expand Down Expand Up @@ -504,14 +513,15 @@ define(function (require, exports, module) {


// Define public API
exports.TAG_NAME = TAG_NAME;
exports.ATTR_NAME = ATTR_NAME;
exports.ATTR_VALUE = ATTR_VALUE;
exports.TAG_NAME = TAG_NAME;
exports.CLOSING_TAG = CLOSING_TAG;
exports.ATTR_NAME = ATTR_NAME;
exports.ATTR_VALUE = ATTR_VALUE;

exports.getTagInfo = getTagInfo;
exports.getTagInfo = getTagInfo;
exports.getTagAttributes = getTagAttributes;
//The createTagInfo is really only for the unit tests so they can make the same structure to
//compare results with
exports.createTagInfo = createTagInfo;
exports.createTagInfo = createTagInfo;
exports.findStyleBlocks = findStyleBlocks;
});