godismyjudge95 / shorthand

A Brackets extension for expanding shorthand css properties in the inline editor.
MIT License
4 stars 5 forks source link

What the status? #3

Open redmunds opened 10 years ago

redmunds commented 10 years ago

Hi Leinardo,

I see that you have this repo started. Awesome!

I don't see a main.js file yet :(

I will have some time next week to help you get the inline editor piece working. If you have anything started, please merge it and I'll take it from there.

Then you can focus on the conversion functions.

Randy

godismyjudge95 commented 10 years ago

I am so sorry I have not gotten to upload anything. I have been so busy with other stuff. Anyways, I did not have anything worth merging in. I was just fiddling around trying to get the hang of how stuff needs to be called, etc. Please let me know if there is anything else I can do. Meanwhile, I will try to get back on the ball working on the main.js

redmunds commented 10 years ago

I'll put together a main.js for you and submit a pull request some time next week.

godismyjudge95 commented 10 years ago

Great! Thanks for your help with this.

redmunds commented 10 years ago

I saw that you merged the inline editor, so now I have some questions about the conversion code.

define(function (require, exports, module) {

    // [snip]

    exports.apiCall1   = apiCall1;
    exports.apiCall2   = apiCall2;
});

Which is loaded where needed like:

    var convert = require("lib/convert");
border: 1px solid black;

It expands to:

border-width: 1px;
border-style: solid;
border-color: black;

Which then expands to:

border-top-width: 1px;
border-right-width: 1px;
border-bottom-width: 1px;
border-left-width: 1px;
border-top-style: solid;
border-right-style: solid;
border-bottom-style: solid;
border-left-style: solid;
border-top-color: black;
border-right-color: black;
border-bottom-color: black;
border-left-color: black;

Unfortunately, brackets does not yet support nested inline editors, so we would have to expand from first all the way to the last in one step.

I haven't looked at the API for converting from shorthand to longhand, and longhand to shorthand, yet. I was thinking about implementing a simple shorthand such as "margin" just to have something to test with :)

I was hoping to get at least 1 shorthand implemented to provide a pattern for others to follow. The Brackets Team is having a couple "innovation days" (today and Monday) and this is one of the things that I decided to work on. I need to give a demo of what I worked on, and the current inline editor is kind of boring :)

Let me know what you think by Monday if possible. Thanks.

godismyjudge95 commented 10 years ago

Agree on the idea for the convert.js module. I could probably implement a test to see if the property is supported. The reason for why border was the first item on the list was because that was the one that made me think of the idea in the first place. If we need to change the priority of which ones to implement first that would be fine. I have no idea why we used the term rules instead of declarations, but I will change that. Thanks for your help!

redmunds commented 10 years ago

I came up with the idea of a "provider" API similar to many of the provider APIs in Brackets. It would work like this:

There would be a ShorthandManager module. Each shorthand provider cold be placed in a separate file (or multiple providers in a single file). ShorthandManager would "require" each file something like:

    var marginProvider = require("providers/margin-provider");

Each provider must implement these API calls:

    convertShorthandToLonghand(shorthand);
    convertLonghandToShorthand(longhand);

ShorthandManager would have a these API calls:

    // InlineShorthandEditor would call this to get provider for a css property name.
    // If property is not supported, then null is returned.
    // This replaces "isPropertySupported()" function I mentioned earlier.
    getProviderForShorthand(shorthandPropName);

    // Each provider would register with this
    registerProvider(this);

If that sounds ok, then I'll take a shot at implementing it.

One important piece of code that you can start looking at is some code to parse the text into CSS properties and values. I think that's going to be the hardest part of the conversion process. Once that is figured out, it should be relatively easy to write the provider code. Brackets has a CSSUtils module that I used in main.js that might be good enough.

This is going to be a really cool Brackets extension.

godismyjudge95 commented 10 years ago

So, let me get this straight, each css property will have its own provider that will register with the ShorthandManager? When we add support for a new property all we have to do is add a new provider to the ShorthandManager and then create that provider? When we need to check whether a property is supported all we have to do is check to see if that provider is available and if not then just return null?

If I am understanding this correctly, I think that would be an excellent way of implementing new properties while still being able to check support quickly.

I have already been looking a bit into the parse code for css and hopefully will have most of it done by the end of the weekend.

Thanks for the help!

godismyjudge95 commented 10 years ago

If the CSSUtils module does not work well enough, we maybe could use some of this: https://github.com/reworkcss/css-parse

Just one of many projects I have found that parse css, but this one seems especially light weight. Let me know what you think.

redmunds commented 10 years ago

I made good progress over the weekend. The only thing left to do is figure out the css parsing.

I could make CSSUtils work, but it wold be a fair amount of work. I looked at css-parse -- it looks great, except that it's a node module. Brackets has a built in node server, but I haven't gotten around to learning how to use it.

Good news. I found a CSS parser that's already built in Brackets: LESS. A pure css document is valid LESS, and it returns the parse tree, so I'm going to just use that. I should have something working soon.

godismyjudge95 commented 10 years ago

Wow! Would never have figured that out on my own... Did not know what Brackets already has a CSS parser. That is great! Let me know what you are working on, that way I do not duplicate work you are already doing...

redmunds commented 10 years ago

I submitted a pull request: https://github.com/LeinardoSmith/shorthand/pull/5

FezVrasta commented 10 years ago

If you guys need help to fix my convert.js just let me know, I've not experience with Brackets extensions and that one was just a sketch to let the project start.

redmunds commented 10 years ago

@LeinardoSmith or @FezVrasta Can one of you try to migrate convert.js to the API I put in place? Take a look at providers/margin.js to see how the API is used. Let me know if the API makes sense and if you have any suggestions about how to improve it. Thanks.

redmunds commented 10 years ago

Also, look at the notes in pull request https://github.com/LeinardoSmith/shorthand/pull/5. If you find that info helpful, it might be worth creating a "How to Contribute" page in the wiki with that info.

redmunds commented 10 years ago

@LeinardoSmith @FezVrasta Hi guys. So, what do you think of the API I put in place? It was merged, so I guess you like it, but I expected at least a little discussion about it. I hope you don't think that I'm trying to take this over.

I thought it made more sense to pass around already parsed objects than raw text. This requires more understanding by the dev, but less requirement for parsing. Let me know -- we can change it to passing just text instead.

I would like to mention this project to the Brackets community to try to get others to help implementing some more shorthand properties, but I was hoping to have a couple more in place by someone other than me :) This would also provide more examples for others to follow.

@FezVrasta I was hoping you could make an attempt to change convert.js to the new format to provide me with some feedback on how you think it works. Let me know if you have any questions.

FezVrasta commented 10 years ago

@redmunds Sorry I've some project I'm working on and I've tried to take a look to your API but honestly it's a bit too much for me to just "read and patch my convert.js", I should study a bit to understand it and I've no time.

For what I've understood you use "providers" to parse the different CSS properties, but in this way seems like my convert.js becomes completely useless, correct me if I'm wrong.

My convert.js was though as a converter that takes the specified property + values and expand/condenses it. Instead your providers are like... oh well I can't understand them.

If you write some kind of documentation about what you've written so I've not to read every single bit to understand the code I can work with you.

Cheers.

redmunds commented 10 years ago

How to Create a New Shorthand Provider

Create New File

Create a new .js file in the providers folder. You can start with this template:

define(function (require, exports, module) {
    "use strict";

    var ShorthandManager    = require("ShorthandManager");

    // Provider Prototypes

    /**
     * @constructor
     */
    function MyProvider() {
    }

    /**
     * Converts a CSS shorthand declaration to a longhand declaration list.
     *
     * If input cannot be converted, then null is returned.
     *
     * AST format is from LESS parser.
     *
     * @ param {name: {string}, value.value[0].value: {Array<{string}>}} decl CSS shorthand declaration
     * @return {Array<{name:{string}, value.value[0].value:{Array<{string}}}>}
     */
    MyProvider.prototype.convertShorthandToLonghand = function (decl) {

    };

    /**
     * Converts a CSS longhand declaration list to a shorthand declaration.
     *
     * If input cannot be converted, then null is returned.
     *
     * AST format is from LESS parser.
     *
     * @ param {Array<{name: {string}, value.value[0].value: {string}}>} decl CSS shorthand declaration
     * @return {name:{string}, value.value[0].value:{string}}
     */
    MyProvider.prototype.convertLonghandToShorthand = function (declList) {

    };

    // Initialize
    var provider = new MyProvider();
    ShorthandManager.registerShorthandProvider("shorthand-property-name", provider);
});

Load New File

In main.js, in the "Load providers" section, add a require statement to load new provider:

    require("providers/myProvider");

where "myProvider" is the name of you new file (without file extension).

Update Class Name

Change "MyProvider" to the name of your new provider class.

Specify CSS Shorthand Property Name

Change "shorthand-property-name" literal string to be the name of the shorthand property.

Implement Conversion Methods

Implement convertShorthandToLonghand and convertLonghandToShorthand methods. See header comments for each function in code template.

The following API calls in ShorthandManager may be useful:

    expandTRBLValues(vals)
    collapseTRBLValues(vals)
    findPropInDecList(propName, declList)
    parseDeclarationList(declListText)
    unparseDeclarationList(declList)
    wsvToArray(str)

If you write any other functions that you think may be useful to others, then add then to ShorthandManager.

Look at some of the other files in the provider folder as an example.

Add License

The text in the LICENSE file should be added to the top of the file.

redmunds commented 10 years ago

@FezVrasta My previous comment explains how to add a new provider. Basically, your "expand" code goes in "convertShorthandToLonghand" and your "condense" code goes in "convertLonghandToShorthand". The only real difference between convert.js and my API is that each shorthand is in a separate class. I separated each shorthand into separate classes because I think that having all shorthand conversion code in a single function will be very difficult to maintain as more shorthand properties are added. I think this will be easier to get others to contribute to. Let me know if you disagree or have any questions.

@LeinardoSmith My previous comment was intended to be an article in wiki markdown format. Not sure where you want to put that article -- it probably best as a wiki page and then link to it from README, or maybe just put it in README.

FezVrasta commented 10 years ago

Thank you for the doc. maybe you could submit it as wiki page of the project.

I will try to work on some providers tomorrow morning. The only question I have is, what kind of input the provider receives?

Thank you for all your support.

redmunds commented 10 years ago

I tried to add it as a wiki page, but I don't think wiki pages are enabled for this repo.

godismyjudge95 commented 10 years ago

@redmunds I can enable the wiki pages. Thank you for writing all of that up! Also, sorry I missed this whole discussion... I have been really busy, but I would love to work more on this. I just need to learn more about how this Bracket stuff works. I am not as handy with javascript as you are so it might take me a bit longer to get a handle on things...

I can work on the background property if you guys don't mind.

redmunds commented 10 years ago

@LeinardoSmith To enable the wiki, I think all you need to do is to go to https://github.com/LeinardoSmith/shorthand and click on the wiki icon on right.

background property support would be awesome -- that's the hardest one to remember for me.

godismyjudge95 commented 10 years ago

Just posted your page and enabled the wiki. I will get started on the background property provider. Let me know if there is anything else I can help with.

godismyjudge95 commented 10 years ago

Ok, so a quick poll. When converting from shorthand to longhand should there be default values for properties not listed? If so, what should those be? Also, should those values be automatically added to the code when converting back to shorthand?

For example:

Shorthand:
     background: blue;

Longhand:
     background-attachment: ?
     background-color: blue;
     background-image: ?
     background-position: ?
     background-repeat: ?
     background-clip: ?
     background-origin: ?
     background-size: ?

or should it be....

Longhand:
     background-attachment: scroll;
     background-color: blue;
     background-image: url("") ;
     background-position: left top;
     background-repeat: no-repeat;
     background-clip: content-box;
     background-origin: content-box;
     background-size: 0 0;

Then converting back should it look like this:

Shorthand:
     background: blue;

Or this?

Longhand:
     background: scroll blue url("") left top / 0 0 no-repeat content-box content-box;

Also, what order do you think they should go in? I did a little research and there seems to be no particular order to this shorthand property... (http://msdn.microsoft.com/en-us/library/ie/ms530722%28v=vs.85%29.aspx https://developer.mozilla.org/en-US/docs/Web/CSS/background) Maybe follow this? http://dev.w3.org/csswg/css-backgrounds/#the-background

Thanks for the help!

redmunds commented 10 years ago

Try to use specs from w3c as they are more agnostic than the browser vendors. I always start here: http://www.w3.org/Style/CSS/current-work

The background spec that you linked to is the latest version, but it's also a draft. The current recommendation is linked off the other page: http://www.w3.org/TR/css3-background/

I always thought that the background property has many order dependencies, but I don't seem to see many.

Of course, the background-position values need to stay in order.

Also, the values for background-origin and background-clip are the same type, so I would think they have to be in a certain order.

Yes, it makes sense to list the default values when you expand to longhand. If possible, you should try to keep track of which longhand values were explicitly specified versus default values so when you convert back to shorthand only values that were explicitly set or were in the original are listed.

godismyjudge95 commented 10 years ago

Would it be too bulky just to use a temporary jquery element to handle the expanding task? e.g. http://stackoverflow.com/questions/5618676/how-to-parse-css-font-shorthand-format#answer-5619682

redmunds commented 10 years ago

Brackets already loads jQuery, so it wouldn't be adding any bulk to use it. That seems like a clever way to do get the longhand values from a shorthand rule, but can you use it to convert back to shorthand? And if you can, do you lose track of what properties were originally specified?

If you have to write code to condense back to shorthand, then that same code is probably what you would need to expand to longhand.

godismyjudge95 commented 10 years ago

Ok... I am having some trouble passing the values correctly to jquery... it seems the css parser is a little messy. Maybe you can figure out why the values are not being recognized properly?

Here is a pastebin:

bg.js -- http://pastebin.com/PUL435ze ShorthandManager.js -- http://pastebin.com/6kENuhTY

redmunds commented 10 years ago

It would be much easier for me to review if you create a branch in your repo and the started a pull request. Then I can pull your branch locally and execute the code, and then put comments directly in your code.

Also, the function expandBGValues() sounds like something that will only ever be used by the background provider, so it shouldn't be a part of the ShorthandManager as it is.

You could make it generic enough for the ShorthandManager by renaming it to something like expandShorthandValues and passing in these additional parameters:

  1. shorthandProp {string} (e.g. "background")
  2. longHandProps {Array<{string}>} (e.g. ["backgroundImage", "backgroundPosition", ... ])
redmunds commented 10 years ago

Forgot to say -- let me know if you need help with starting a branch or a pull request.

godismyjudge95 commented 10 years ago

Done. Do I need to merge the pull request? (note: this is not stable yet)

redmunds commented 10 years ago

No, do not merge until stable. I will take a look.

godismyjudge95 commented 10 years ago

Ok. Thanks. I noted a few other problems in the pull request. Maybe fixing that one problem will fix the rest. Thanks for the help.