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

Return the css property value range in getInfoAtPos() api. #7390

Merged
JeffryBooher merged 10 commits into from
Apr 5, 2014
Merged

Return the css property value range in getInfoAtPos() api. #7390

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e489b36
Return the property value range in getInfoAtPos() api.
RaymondLim Apr 2, 2014
d82b043
simple unit test
JeffryBooher Apr 4, 2014
045f312
'enhanced' the test data
JeffryBooher Apr 4, 2014
241352b
updated unit test data
JeffryBooher Apr 4, 2014
0b6747d
Merge branch 'master' into rlim/range-in-css-info
JeffryBooher Apr 4, 2014
a4465a6
update range unit tests
JeffryBooher Apr 4, 2014
14987bb
Fix all CSS Context Info unit tests by adding range info.
RaymondLim Apr 5, 2014
d5b8102
rtc: spelling errors
JeffryBooher Apr 5, 2014
870e99a
Handle ranges for empty property values and fix some more unit tests.
RaymondLim Apr 5, 2014
99dbf8c
Fix another feedback.
RaymondLim Apr 5, 2014
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
74 changes: 64 additions & 10 deletions src/language/CSSUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ define(function (require, exports, module) {
EditorManager = require("editor/EditorManager"),
HTMLUtils = require("language/HTMLUtils"),
ProjectManager = require("project/ProjectManager"),
TokenUtils = require("utils/TokenUtils");
TokenUtils = require("utils/TokenUtils"),
_ = require("thirdparty/lodash");

// Constants
var SELECTOR = "selector",
Expand Down Expand Up @@ -128,20 +129,25 @@ define(function (require, exports, module) {
* @param {Array.<string>=} values An array of property values
* @param {boolean=} isNewItem If this is true, then the value in index refers to the index at which a new item
* is going to be inserted and should not be used for accessing an existing value in values array.
* @param {{start: {line: number, ch: number},
* end: {line: number, ch: number}}=} range A range object with a start position and an end position
* @return {{context: string,
* offset: number,
* name: string,
* index: number,
* values: Array.<string>,
* isNewItem: boolean}} A CSS context info object.
* isNewItem: boolean,
* range: {start: {line: number, ch: number},
* end: {line: number, ch: number}}}} A CSS context info object.
*/
function createInfo(context, offset, name, index, values, isNewItem) {
function createInfo(context, offset, name, index, values, isNewItem, range) {
var ruleInfo = { context: context || "",
offset: offset || 0,
name: name || "",
index: -1,
values: [],
isNewItem: (isNewItem) ? true : false };
isNewItem: (isNewItem) ? true : false,
Copy link
Copy Markdown
Contributor

@JeffryBooher JeffryBooher Apr 5, 2014

Choose a reason for hiding this comment

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

why the conditional? can't isNewItem just just be assigned to the argument coming in? you could do isNewItem: (isNewItem === true) which is more performant since it's just a simple math eval

range: range };

if (context === PROP_VALUE || context === SELECTOR || context === IMPORT_URL) {
ruleInfo.index = index;
Expand All @@ -165,7 +171,9 @@ define(function (require, exports, module) {
* name: string,
* index: number,
* values: Array.<string>,
* isNewItem: boolean}} A CSS context info object.
* isNewItem: boolean,
* range: {start: {line: number, ch: number},
* end: {line: number, ch: number}}}} A CSS context info object.
*/
function _getPropNameInfo(ctx) {
var propName = "",
Expand Down Expand Up @@ -335,6 +343,43 @@ define(function (require, exports, module) {
return propValues;
}

/**
* @private
* Return a range object with a start position and an end position after
* skipping any whitespaces and all separators used before and after a
* valid property value.
*
* @param {editor:{CodeMirror}, pos:{ch:{string}, line:{number}}, token:{object}} startCtx context
* @param {editor:{CodeMirror}, pos:{ch:{string}, line:{number}}, token:{object}} endCtx context
* @return {{start: {line: number, ch: number},
* end: {line: number, ch: number}}} A range object.
*/
function _getRangeForPropValue(startCtx, endCtx) {
var range = { "start": {},
"end": {} };

// Skip the ":" and any leading whitespace
while (TokenUtils.moveNextToken(startCtx)) {
if (startCtx.token.string.trim().length > 0) {
Copy link
Copy Markdown
Contributor

@JeffryBooher JeffryBooher Apr 5, 2014

Choose a reason for hiding this comment

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

I think that this might be better while (TokenUtils.moveNextToken(startCtx) && !startCtx.token.string.trim()); but I'm not sure if crockford will like it.

another thing that i'm wondering if TokenUtils.moveNextToken() returns false then we fall through and we're using the property name as the thing returned. Perhaps that needs a unit test in the case of a malformed property at the end of the file.

Copy link
Copy Markdown
Contributor Author

@RaymondLim RaymondLim Apr 5, 2014

Choose a reason for hiding this comment

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

Yes, linter will complain on the empty block. And you're mistaken with the direction. We're scanning forwards and not backwards here.

break;
}
}

// Skip the trailing whitespace and property separators.
while (endCtx.token.string === ";" || endCtx.token.string === "}" ||
endCtx.token.string.trim().length === 0) {
Copy link
Copy Markdown
Contributor

@JeffryBooher JeffryBooher Apr 5, 2014

Choose a reason for hiding this comment

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

you can just say !endCtx.token.string.trim() since the resultant string is considered falsy if it's empty

TokenUtils.movePrevToken(endCtx);
}

range.start = _.clone(startCtx.pos);
range.start.ch = startCtx.token.start;

range.end = _.clone(endCtx.pos);
range.end.ch = endCtx.token.end;

return range;
}

/**
* @private
* Returns a context info object for the current CSS style rule
Expand All @@ -345,7 +390,9 @@ define(function (require, exports, module) {
* name: string,
* index: number,
* values: Array.<string>,
* isNewItem: boolean}} A CSS context info object.
* isNewItem: boolean,
* range: {start: {line: number, ch: number},
* end: {line: number, ch: number}}}} A CSS context info object.
*/
function _getRuleInfoStartingFromPropValue(ctx, editor) {
var propNamePos = $.extend({}, ctx.pos),
Expand All @@ -361,7 +408,8 @@ define(function (require, exports, module) {
canAddNewOne = false,
testPos = {ch: ctx.pos.ch + 1, line: ctx.pos.line},
testToken = editor._codeMirror.getTokenAt(testPos, true),
propName;
propName,
range;

// Get property name first. If we don't have a valid property name, then
// return a default rule info.
Expand Down Expand Up @@ -413,13 +461,15 @@ define(function (require, exports, module) {
forwardCtx = TokenUtils.getInitialContext(editor._codeMirror, forwardPos);
propValues = propValues.concat(_getSucceedingPropValues(forwardCtx, lastValue));

range = _getRangeForPropValue(backwardCtx, forwardCtx);

// If current index is more than the propValues size, then the cursor is
// at the end of the existing property values and is ready for adding another one.
if (index === propValues.length) {
canAddNewOne = true;
}

return createInfo(PROP_VALUE, offset, propName, index, propValues, canAddNewOne);
return createInfo(PROP_VALUE, offset, propName, index, propValues, canAddNewOne, range);
}

/**
Expand All @@ -432,7 +482,9 @@ define(function (require, exports, module) {
* name: string,
* index: number,
* values: Array.<string>,
* isNewItem: boolean}} A CSS context info object.
* isNewItem: boolean,
* range: {start: {line: number, ch: number},
* end: {line: number, ch: number}}}} A CSS context info object.
*/
function _getImportUrlInfo(ctx, editor) {
var propNamePos = $.extend({}, ctx.pos),
Expand Down Expand Up @@ -501,7 +553,9 @@ define(function (require, exports, module) {
* name: string,
* index: number,
* values: Array.<string>,
* isNewItem: boolean}} A CSS context info object.
* isNewItem: boolean,
* range: {start: {line: number, ch: number},
* end: {line: number, ch: number}}}} A CSS context info object.
*/
function getInfoAtPos(editor, constPos) {
// We're going to be changing pos a lot, but we don't want to mess up
Expand Down
55 changes: 55 additions & 0 deletions test/spec/CSSUtils-test-files/ranges.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/* */

.foo {
shape-inside: circle(0
at
0 0
);

shape-inside: circle (0px
at
0px

0px
);

shape-inside: polygon(0 0,
100px

0,
100px 100px


);


shape-inside:
polygon(


nonzero,
0 0,
100px
0,
100px

100px
);
}



@keyframes colorize {
0% {
-webkit-filter: grayscale(100%);
}
100% {
-webkit-filter:


grayscale
( 0 % )

;
}
}
Loading
⚡