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

[OPEN] Update Less to 1.4.2 #4476

Merged
merged 9 commits into from
Sep 16, 2013
Merged

Conversation

WebsiteDeveloper
Copy link
Contributor

No description provided.

@ghost ghost assigned gruehle Jul 16, 2013
@@ -40,23 +40,23 @@
/* LESS imports */

// Bootstrap @ v.2.3.1
@import url(bootstrap/bootstrap.less);
@import "bootstrap/bootstrap.less";
Copy link
Member

Choose a reason for hiding this comment

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

Removing url() doesn't seem necessary. Why was that change made?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because a crash happened when not removing url().

Copy link
Member

Choose a reason for hiding this comment

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

Interesting.. I tried removing url() and everything worked fine for me. I also updated the branch to the latest master, so I'm not sure if that made a difference or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for me there seemed to happen some kind of parse error in less.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where are you seeing the error? I switched back to using url() and I don't see any problems. If you are still seeing problems, can you try using both url() and quotes? For example:

@import url("bootstrap/bootstrap.less");

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you at the latest version of master? We upgraded CEF recently (in Sprint 31), so maybe that's the difference.

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 just rebuilt my shell with the latest code to be sure, but the crash still happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion this has to do with how less resolves the relative paths which could also be the reason for the unit test failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could try either the rootpath or relativeUrls options described here: http://lesscss.org/#usage

Otherwise, file an issue with LESS. I'm sure they'd like to hear about it and may even provide a workaround.

Copy link
Contributor

Choose a reason for hiding this comment

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

With url and quotes it works but without i get a parse error.

I missed this statement you made above. If it works with both url() and quotes, then make that change and I am OK with it.

@gruehle
Copy link
Member

gruehle commented Jul 17, 2013

@WebsiteDeveloper Thanks. Are you aware of any bugs in 1.4.1? I did some quick sanity testing with your branch and everything seemed okay.

@WebsiteDeveloper
Copy link
Contributor Author

@gruehle i am currently not aware of any bugs in 1.4.1. Actually i have been working with this branch a few days and didn't notice any bugs yet.

@TuckerWhitehouse
Copy link
Contributor

I've also been using the less 1.4.1 compiler for the past few days and haven't noticed any issues.

On another note, If you enabled the "strictMath" mode (which will be enabled by default in the future), the UI loads, but the sidebar is rather broken...
screen shot 2013-07-17 at 7 05 34 pm

@WebsiteDeveloper
Copy link
Contributor Author

@TuckerWhitehouse i think i'll look into that and open another subsequent pull request

@WebsiteDeveloper
Copy link
Contributor Author

@gruehle any news on this?

@gruehle
Copy link
Member

gruehle commented Aug 4, 2013

@WebsiteDeveloper - Sorry, I've been out on vacation this week, and will be out next week, too. I'm going to tag this as [OPEN] to see if someone else can get this merged in before I return.

@WebsiteDeveloper
Copy link
Contributor Author

@gruehle no problem :) enjoy your vacation.

@WebsiteDeveloper
Copy link
Contributor Author

@gruehle are you in again?

@ghost ghost assigned redmunds Sep 1, 2013
@redmunds
Copy link
Contributor

redmunds commented Sep 3, 2013

@WebsiteDeveloper I can take a look at this one.

@redmunds
Copy link
Contributor

redmunds commented Sep 3, 2013

@WebsiteDeveloper I'm ready to review this one -- can you merge the latest master into your branch?

@WebsiteDeveloper
Copy link
Contributor Author

@redmunds merged with master.

@redmunds
Copy link
Contributor

redmunds commented Sep 4, 2013

This unit test is failing: Integration > Extension Utils > "should attach LESS style sheets":

Error: Expected '/* basic.less */
#project-title {
  font-size: 66px;
}
' to be '/* basic.less */
#project-title {
  font-weight: 800;
}
#project-title {
  font-size: 66px;
}
'.
    at new jasmine.ExpectationResult (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:102:32)
    at null.toBe (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1194:29)
    at null.<anonymous> (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/spec/ExtensionUtils-test.js:134:40)
    at jasmine.Block.execute (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1024:15)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1842:31)
    at onComplete (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1838:18)
    at jasmine.WaitsForBlock.execute (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2322:5)
    at file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2336:12

@redmunds
Copy link
Contributor

redmunds commented Sep 4, 2013

Done with initial review.

@WebsiteDeveloper
Copy link
Contributor Author

@redmunds by the way if i put an url around the current import line in basic.less.
like that url("sub dir/fourth.less"); the unit test don't fail anymore. This is very weird actually.

@WebsiteDeveloper
Copy link
Contributor Author

@redmunds i pushed a few changes.

@TomMalbran
Copy link
Contributor

@WebsiteDeveloper It looks like tou still need to merge with master and fix the conflict issues.

@WebsiteDeveloper
Copy link
Contributor Author

@TomMalbran will do so thanks for the heads up.

@WebsiteDeveloper
Copy link
Contributor Author

@redmunds @TomMalbran fixed merge conflict.

@redmunds
Copy link
Contributor

redmunds commented Sep 8, 2013

@WebsiteDeveloper This is looking good, but there's a unit test failing in this branch that's not failing in master. Maybe just need to sync with master again?

Unit > HighlightAgent > "should toggle the highlight via a command"

Error: Expected spy showHighlight to have been called.
    at new jasmine.ExpectationResult (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:102:32)
    at null.toHaveBeenCalled (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1194:29)
    at null.<anonymous> (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/spec/LiveDevelopment-test.js:299:65)
    at jasmine.Block.execute (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1024:15)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1842:31)
    at jasmine.Queue.start (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1795:8)
    at jasmine.Spec.execute (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2122:14)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1842:31)
    at jasmine.Queue.start (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1795:8)
    at jasmine.Suite.execute (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2267:14)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1842:31)

@WebsiteDeveloper
Copy link
Contributor Author

@redmunds i couldn't figure it out until now. Just wanted to lett you know that i will be away until next week.
Maybe you can figure out whats the matter with the unit test, if you have some spare time ;)

@redmunds
Copy link
Contributor

@WebsiteDeveloper I'm hoping that is fixed with pull request #5115, so try merging with latest master code before spending any time on it.

@WebsiteDeveloper
Copy link
Contributor Author

@redmunds i fixed the unit test failure. this one just failed because less wasn't correctly included.

@redmunds
Copy link
Contributor

@WebsiteDeveloper I'm also noticing a reference to 'src/thirdparty/less-1.3.3.min.js' in brackets/Gruntfile.js that needs to be updated. Thanks.

@WebsiteDeveloper
Copy link
Contributor Author

@redmunds changes pushed.

@redmunds
Copy link
Contributor

Thanks! Merging.

redmunds added a commit that referenced this pull request Sep 16, 2013
@redmunds redmunds merged commit 939d97f into adobe:master Sep 16, 2013
@WebsiteDeveloper WebsiteDeveloper deleted the less-upgrade branch February 1, 2014 16:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants