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

Unit tests for LESS/SCSS #8905

Closed
wants to merge 28 commits into from
Closed

Unit tests for LESS/SCSS #8905

wants to merge 28 commits into from

Conversation

RaymondLim
Copy link
Contributor

Also fixes for #8894 and #8895.

RaymondLim and others added 20 commits August 22, 2014 11:10
Fix some issues with the selectors in the final CSS form.
…`&` issue.

Fix a JSLint error in unit test file and adjust a string for group selector verification.
@JeffryBooher
Copy link
Contributor

@RaymondLim there are other changes here too. Does this PR cover more than just adding unit tests?

@ingorichter
Copy link
Contributor

Looks like changes to fix issues and to please the test cases.

@RaymondLim
Copy link
Contributor Author

I created this pull request branch from rlim/parse-less-and-scss branch before puill request #8844 landed in master. So all changes in #8844 also show up in this pull request.

I just merge master to this branch and after pushing the merge all those changes in #8844 no longer show up. So it's ready to review. You will see some changes in CSSUtils.js -- those changes are fixes for #8894 and #8895.

});
return finalSelectorArray.join(", ");
}

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 moved out these two functions from findSelectorAtDocumentPos scope to the module scope so that I can call _getSelectorInFinalCSSForm from other functions in the module. The only change that I made in _getSelectorInFinalCSSForm is ps = cs.replace("&", ps); --> ps = cs.replace(/&/g, ps);.

* Returns trimmed selector if it is not an at-rule, or null if it starts with @.
*
* @param {string} selector
* @return {?string}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be @return {string}. The ? indicates nullable, and return value can be empty, but not null.

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 point!

@redmunds
Copy link
Contributor

redmunds commented Sep 4, 2014

Done with initial review.

@ingorichter
Copy link
Contributor

@redmunds I'm done with adding tests.

@RaymondLim
Copy link
Contributor Author

@redmunds All changes are pushed and merge conflicts are solved. Ready for review.

});
});

// https://github.com/adobe/brackets/issues/8895
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent this comment same as here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Argh... Thanks ;-)

@redmunds
Copy link
Contributor

redmunds commented Sep 9, 2014

Done with second review. I have a few questions, and there's a little cleanup to do.

@RaymondLim
Copy link
Contributor Author

@redmunds Thanks for your reviews and questions. @ingorichter will take care of cleanup as they're in CSS inline editor tests.

@redmunds
Copy link
Contributor

@RaymondLim @ingorichter Looks good. Squash the commits and I'll merge it.

@RaymondLim
Copy link
Contributor Author

Closing for a new squashed pr.

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

Successfully merging this pull request may close these issues.

4 participants