Refactored ExtensionUtils and added LESS support #2126

Merged
merged 3 commits into from Dec 12, 2012

Conversation

Projects
None yet
3 participants
@DennisKehrig
Contributor

DennisKehrig commented Nov 15, 2012

As suggested by @peterflynn

@ghost ghost assigned peterflynn Nov 15, 2012

@ghost ghost assigned redmunds Dec 5, 2012

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 5, 2012

Contributor

@peterflynn This was assigned to you, but it hasn't been touched in weeks, so I figured it was fair game :)

Contributor

redmunds commented Dec 5, 2012

@peterflynn This was assigned to you, but it hasn't been touched in weeks, so I figured it was fair game :)

src/utils/ExtensionUtils.js
+ * @param {!string} css CSS code to use as the tag's content
+ * @return {!HTMLStyleElement} The generated HTML node
+ **/
+ function addInlineStyleSheet(css) {

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 5, 2012

Contributor

<style> tags are known as "embedded" style sheets. "Inline" styles are specified in the style attribute of html tags (and is not a style sheet). So, the name of this function should be changed to something like: addEmbeddedStyleSheet().

@redmunds

redmunds Dec 5, 2012

Contributor

<style> tags are known as "embedded" style sheets. "Inline" styles are specified in the style attribute of html tags (and is not a style sheet). So, the name of this function should be changed to something like: addEmbeddedStyleSheet().

This comment has been minimized.

Show comment Hide comment
@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Very good point, I agree!

@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Very good point, I agree!

src/utils/ExtensionUtils.js
+ * @param {!string} url URL to a style sheet
+ * @return {!HTMLLinkElement} The generated HTML node
+ **/
+ function addStyleSheetReference(url) {

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 5, 2012

Contributor

To be more consistent with "addEmbeddedStyleSheet" maybe this should be called "addLinkedStyleSheet" ?

@redmunds

redmunds Dec 5, 2012

Contributor

To be more consistent with "addEmbeddedStyleSheet" maybe this should be called "addLinkedStyleSheet" ?

This comment has been minimized.

Show comment Hide comment
@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Sounds good.

@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Sounds good.

src/utils/ExtensionUtils.js
+ if (url.slice(-5) === ".less") {
+ parseLessCode(content, url).done(function (css) {
+ result.resolve(addInlineStyleSheet(css));
+ }).fail(result.reject);

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 5, 2012

Contributor

I think the .done() and .fail() callbacks should be vertically aligned to make this code easier to read. I haven't verified this passes JSLint, but something like this:

parseLessCode(content, url)
    .done(function (css) {
        result.resolve(addInlineStyleSheet(css));
    })
    .fail(result.reject);
@redmunds

redmunds Dec 5, 2012

Contributor

I think the .done() and .fail() callbacks should be vertically aligned to make this code easier to read. I haven't verified this passes JSLint, but something like this:

parseLessCode(content, url)
    .done(function (css) {
        result.resolve(addInlineStyleSheet(css));
    })
    .fail(result.reject);

This comment has been minimized.

Show comment Hide comment
@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Okay, I'll do that, if it passes in JSLint.

@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Okay, I'll do that, if it passes in JSLint.

src/utils/ExtensionUtils.js
+ * @param {!string} url URL of a resource
+ * @return {!$.Promise} A promise object that is resolved with the contents of the requested resource
+ **/
+ function loadUrl(url) {

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 5, 2012

Contributor

I don't think this function does enough to be exported by this module. I think it should be removed from the exports to simplify the API.

Actually, I am not sure it's even worth having a function at all.

@redmunds

redmunds Dec 5, 2012

Contributor

I don't think this function does enough to be exported by this module. I think it should be removed from the exports to simplify the API.

Actually, I am not sure it's even worth having a function at all.

This comment has been minimized.

Show comment Hide comment
@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

This could potentially become a place for URL rewriting, should that be necessary in a hosted version. But it probably won't be used consistently anyway and it's too speculative to make part of an official API, I agree.

@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

This could potentially become a place for URL rewriting, should that be necessary in a hosted version. But it probably won't be used consistently anyway and it's too speculative to make part of an official API, I agree.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 5, 2012

Contributor

Done with initial review.

Contributor

redmunds commented Dec 5, 2012

Done with initial review.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 5, 2012

Contributor

Also, add at least 1 unit test to test/spec/ExtensionUtils-test.js for loading a .less file. I think that will test all of the API calls, but you're welcome to add any other unit test that you think will be helpful.

Contributor

redmunds commented Dec 5, 2012

Also, add at least 1 unit test to test/spec/ExtensionUtils-test.js for loading a .less file. I think that will test all of the API calls, but you're welcome to add any other unit test that you think will be helpful.

@DennisKehrig

This comment has been minimized.

Show comment Hide comment
@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Thanks, @redmunds! Great job!

Contributor

DennisKehrig commented Dec 12, 2012

Thanks, @redmunds! Great job!

Added a LESS-based unit test for ExtensionUtils.loadStyleSheet
Removed the loadUrl function
Renamed addInlineStyleSheet to addEmbeddedStyleSheet
Renamed addStyleSheetReference to addLinkedStyleSheet
Aligned .done and .fail differently
@DennisKehrig

This comment has been minimized.

Show comment Hide comment
@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

@redmunds All done, back to you :)

Contributor

DennisKehrig commented Dec 12, 2012

@redmunds All done, back to you :)

@@ -0,0 +1,7 @@
+/* basic.css */

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 12, 2012

Contributor

Filename in comment doesn't match actual filename

@redmunds

redmunds Dec 12, 2012

Contributor

Filename in comment doesn't match actual filename

This comment has been minimized.

Show comment Hide comment
@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Thanks, overlooked that one

@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Thanks, overlooked that one

@@ -0,0 +1,5 @@
+// HighASCII_été.less

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 12, 2012

Contributor

It looks like you copied this from one of my test cases that got changed, but the filename in comment should match actual filename

@redmunds

redmunds Dec 12, 2012

Contributor

It looks like you copied this from one of my test cases that got changed, but the filename in comment should match actual filename

This comment has been minimized.

Show comment Hide comment
@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Indeed, I did. I wasn't sure whether this was supposed to test whether exotic characters throw of some part of the process.

@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Indeed, I did. I wasn't sure whether this was supposed to test whether exotic characters throw of some part of the process.

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 12, 2012

Contributor

The high ascii version was the original filename, but github.com couldn't handle it. I renamed the file to low ascii, but forgot to change the comment. :(

@redmunds

redmunds Dec 12, 2012

Contributor

The high ascii version was the original filename, but github.com couldn't handle it. I renamed the file to low ascii, but forgot to change the comment. :(

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 12, 2012

Contributor

Looks good except for a couple comments.

Contributor

redmunds commented Dec 12, 2012

Looks good except for a couple comments.

+ var $projectTitle = testWindow.$("#project-title");
+ var fontVariant = $projectTitle.css("letter-spacing");
+ expect(fontVariant).toEqual("9px");
+ });

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 12, 2012

Contributor

One more comment: This code should be broken into a separate it() { ... } block so it shows up as a new test (not just adding on to the existing one).

@redmunds

redmunds Dec 12, 2012

Contributor

One more comment: This code should be broken into a separate it() { ... } block so it shows up as a new test (not just adding on to the existing one).

This comment has been minimized.

Show comment Hide comment
@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Okay, I was following the "putting everything in 1 test so it runs faster" comment

@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Okay, I was following the "putting everything in 1 test so it runs faster" comment

@DennisKehrig

This comment has been minimized.

Show comment Hide comment
@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Round three!

Contributor

DennisKehrig commented Dec 12, 2012

Round three!

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Dec 12, 2012

Contributor

Looks good. Merging.

Contributor

redmunds commented Dec 12, 2012

Looks good. Merging.

redmunds added a commit that referenced this pull request Dec 12, 2012

Merge pull request #2126 from DennisKehrig/dk/extension-utils
Refactored ExtensionUtils and added LESS support

@redmunds redmunds merged commit 51c0b35 into adobe:master Dec 12, 2012

@DennisKehrig

This comment has been minimized.

Show comment Hide comment
@DennisKehrig

DennisKehrig Dec 12, 2012

Contributor

Awesome, thank you!

Contributor

DennisKehrig commented Dec 12, 2012

Awesome, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment