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

Add CSS Shapes Editor extension to Brackets#7384

Merged
RaymondLim merged 19 commits intoadobe:masterfrom
oslego:shapes-editor
Apr 4, 2014
Merged

Add CSS Shapes Editor extension to Brackets#7384
RaymondLim merged 19 commits intoadobe:masterfrom
oslego:shapes-editor

Conversation

@oslego
Copy link
Copy Markdown
Contributor

@oslego oslego commented Apr 2, 2014

@RaymondLim, @redmunds, @njx please review / track.

@RaymondLim RaymondLim self-assigned this Apr 2, 2014
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be "set to true to avoid ..."?

@larz0
Copy link
Copy Markdown
Member

larz0 commented Apr 2, 2014

@oslego could you use #00a2ff for the blue? I used the wrong blue in the PSD I gave you. Sorry about that!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: attepts

Conflicts:
	src/extensions/default/CSSShapesEditor/LiveEditorLocalDriver.js
	src/extensions/default/CSSShapesEditor/Model.js
	src/extensions/default/CSSShapesEditor/main.js
	src/extensions/default/CSSShapesEditor/thirdparty/CSSShapesEditor.js
@RaymondLim
Copy link
Copy Markdown
Contributor

@oslego Can you either remove or comment out all the console.log in your files?

@oslego
Copy link
Copy Markdown
Contributor Author

oslego commented Apr 3, 2014

@RaymondLim will do. Had them in as a means to know that the injected editor was getting the right commands.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You mean Mitigates, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If these are public methods, the functions shouldn't have a dangling underscore.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be {$.Promise=}.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we remove "clip-path" in this array to match the one used in main.js?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The one in main.js acts like a guard. I think we should keep this intact here, so that when the time does come to support unprefixed clip-path, just the guard needs adjusting. I believe that will avoid confusion.

@RaymondLim
Copy link
Copy Markdown
Contributor

Done with final review. Just have some minor nits. I'll be merging soon even if you can't make these changes.

@oslego
Copy link
Copy Markdown
Contributor Author

oslego commented Apr 4, 2014

  • updated col in negative match test; props used to have prefixes and col 27 was within the property.
  • remove 2 negative match tests: these are treated by the standalone editor. It will throw error / infer new shape from the element.

These negative tests will pass because CodeMirror doesn't match properties with values. fake-polygon() is perfectly fine for CM. The shapes editor treats this case.

These tests used to pass because of other reasons (e.g.: prefixes)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a duplicate of the one above.

@oslego
Copy link
Copy Markdown
Contributor Author

oslego commented Apr 4, 2014

@larz0 Colors changed with RGBA correspondent. Hope you noticed. It's been a stream of changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here, since you need to return the promise object

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Isn't deferred.reject() returning a failed promise already?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Promise is the same as a Deferred object but without the methods that allow you to change the state. I am saying it because the API says that it returns a Promise and not a Deferred.

@RaymondLim
Copy link
Copy Markdown
Contributor

Merging now for others to do scenario testing. We'll fix some outstanding nits later.

RaymondLim added a commit that referenced this pull request Apr 4, 2014
Add CSS Shapes Editor extension to Brackets
@RaymondLim RaymondLim merged commit 1a950d0 into adobe:master Apr 4, 2014
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nonexistent URL

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not public yet. The full source will surface soon.

@larz0
Copy link
Copy Markdown
Member

larz0 commented Apr 4, 2014

Thanks @oslego! 👍

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.

6 participants