Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aulx.CSS constructor, CSS State Machine, and much more. #5

Merged
merged 14 commits into from
Aug 15, 2013

Conversation

grssam
Copy link
Contributor

@grssam grssam commented Jul 29, 2013

This pull request contains:

  • Aulx.CSS constructor with a Aulx.css static method too.
  • CSS State machine to detect where them cursor at.
  • Intensive map of CSS property vs value keywords.
  • More tests for property state detection
  • Combines aulx.js into aulx-ui.js so that aulx-ui.js should be the only one file to use in a browser
  • CSS Value Autocompletion.
  • CSS Selector suggestion based on the DOM of the global passed to Aulx

@@ -706,6 +706,6 @@ UnicodeRangeToken.prototype.contains = function(code) {

// Exportation.
// TODO: also export the various tokens objects?
exports.tokenize = tokenize;
exports.CSS.tokenize = tokenize;
Copy link
Owner

Choose a reason for hiding this comment

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

Can't do that, tokenizer.js is an upstream library. Updating it (which I'm told we should do in about a month) would break this.

Use UMD, similar to how we do with Aulx and Esprima's tokenizer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its just a one line change. Also, when we update from upstream, we can make this change again. I do not want to complicate things using the custom made wrapper. Or else, we can remove this change and access the tokenizer via Aulx.tokenize path instead of Aulx.CSS.tokenize

Copy link
Owner

Choose a reason for hiding this comment

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

When I say "can't do that", I mean it. If you want, I'll do it when I find time.

Great work, though.

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 am confused now.

Had to remove white space stripping as there was no way to distinguish
selectors like "a.class" from "a .class".
@grssam
Copy link
Contributor Author

grssam commented Aug 3, 2013

That's it for now. I will not be having internet access to be development box for like 10 days now so this pull request is complete. Next pull request will contain demo site upgrades to show up both JS and CSS completion in action. And regression fixes if any. Also, I have to add tests to see if selector state is determined properly.

Then later on I will see if attribute and value completion is possible too.

@espadrine espadrine merged commit 27557f4 into espadrine:master Aug 15, 2013
@espadrine
Copy link
Owner

A few things.

  • I played around with the CSS autocompletion, and it's exceedingly magnificent.
  • Thanks a lot for the CSS tests. The number of tests I had before was as ridiculous as the simplicity of the algorithm.
  • It's great that you went the extra mile and connected the CSS system to dynamically find information from the global. I didn't play with that, but I'm eager to see how it feels to write normal projects with it.
  • I told you once before; I really wish commits like bc01f5c that break git bisect didn't exist. A commit that breaks things is a commit I shouldn't accept. In this case, a minority of commits actually pass all the tests. This makes reviewing harder.
  • On your tip, aulx.js has Windows newlines (that is, CR-LF). This project uses Unix newlines. Not sure how that happened though, but it's rather bad.
  • If a change you do requires people to always perform a task whenever they change a piece of code, put a comment describing what task they must do. You forgot to add a comment while changing make.js. (I removed the need to perform that task, which I believe is even better.)
  • You changed getContext to resolveContext, which makes the JS completer have a different terminology than the CSS part. It may surprise future code readers. I didn't change it because I trust you have a reason.

@grssam
Copy link
Contributor Author

grssam commented Aug 15, 2013

Please match the points to the questions:

  • Thanks.
  • Yeah.
  • I am still to revamp the demo page. Then I will showcase the CSS completion too.
  • I am not good at backing-out-then-recommiting kind of stuff with git. Sorry for that.
  • Even I am confused how that happened. I guess it is because of the new files that I added have windows newlines.
  • Not sure what you mean here.
  • It is not a biggy, it can be changed back, The idea here was that not the method's returned object is not actually used in the code as it resolves the context and set it to |this| object which is read later on.

@espadrine
Copy link
Owner

On Thu, Aug 15, 2013 at 7:23 PM, Girish Sharma notifications@github.com wrote:

Not sure what you mean here.

When copy-and-pasting aulx.js into aulx-ui.js in make.js, you
implicitly require every modification of aulx.js' bundle to be
mirrored in aulx-ui.js' bundle.

It is not a biggy, it can be changed back, The idea here was that not the method's returned object is not actually used in the code as it resolves the context and set it to |this| object which is read later on.

Huh, I see. I might want to change JS's getContext() to resolveContext() then.

@grssam
Copy link
Contributor Author

grssam commented Aug 15, 2013

On Fri, Aug 16, 2013 at 1:06 AM, Thaddee Tyl notifications@github.comwrote:

On Thu, Aug 15, 2013 at 7:23 PM, Girish Sharma notifications@github.com
wrote:

Not sure what you mean here.

When copy-and-pasting aulx.js into aulx-ui.js in make.js, you
implicitly require every modification of aulx.js' bundle to be
mirrored in aulx-ui.js' bundle.

I see, maybe we can add the whole aulx.js in the dependency list of
aulx-ui.js instead of building it up via the dependency route ;

It is not a biggy, it can be changed back, The idea here was that not the
method's returned object is not actually used in the code as it resolves
the context and set it to |this| object which is read later on.

Huh, I see. I might want to change JS's getContext() to resolveContext()
then.


Reply to this email directly or view it on GitHubhttps://github.com//pull/5#issuecomment-22724191
.

Girish Sharma
B.Tech(H), Civil Engineering,
Indian Institute of Technology, Kharagpur

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

Successfully merging this pull request may close these issues.

None yet

2 participants