Refactoring

[leobalter]

This is a huge refactoring coming in. Now from #49 I was able to follow the basics of ArteJS usage so it's being easier to refactor every API feature by TDD.

Status:

• Core (it does exist)
• Core Options
• Core methods
• Custom Events
• Commands for plain text editor
• Commands for rich text editor (will probably require some range selection skills)
• Toolbar
• Toolbar buttons
• Plugins
• Documentation review

Some features were changed as a proposal:

• Arte is now an exposed class, so you can create your arte object like this: var arte = new Arte( element, { options } );, it will return the created elemtent as arte.element and the given element container as arte.container;
  • This way allow us to use methods from a stored variable, like arte.bold();, arte.destroy(), and arte.triggerEvent("foo"), e.g.
  • All the commands should be called from this stored object, there're not longer Arte("bold").
• the instantiated arte is no longer attached to the element's data, avoiding a big memory issue
• events namespace were replaced from on to arte- to flag them as proper Arte events, some of them are still connected to the Arte editor common events, e.g.: click on the element triggers arte-click on the arte object.
• rangy will probably be excluded from the external dependencies

I can't say yet how smaller or faster the code is, but it is being successful so far and it's already compatible with any jQuery version from 1.7.2 to 1.11.x.

[gshenar]

these values (such as font-weight) will probably need to live in the configuration right? also it looks like commands need to support toggling classes on the elements as well - https://github.com/vistaprint/ArteJS/blob/master/src/editor/core/Commands.js#L198

[leobalter]

these values (such as font-weight) will probably need to live in the configuration right?

The configuration is creating a big object to live on the memory for no re-usability. There's no need so far to use it, as fontWeight won't be changed for anything else in any expected future.

looks like commands need to support toggling classes on the elements as well

That's something we can add to the tests (even if it fails in the start) to add the feature later. This refactoring is still in progress and it is following the current documentation.

I'll write documentation for this class toggle method and ask you to review. Doing that we can ship all the necessary features.

[gshenar]

although the configuration is a big object there are many places around the code base that need to have access to these configurations - for example the state detector plugin - https://github.com/vistaprint/ArteJS/blob/master/src/editor/plugins/StateDetector.js#L68

[leobalter]

good, but this need to be progressive, step by step with docs, tests and code. If that is required by the state detector plugin, we add the feature.

[gshenar]

I've got a few questions about removing the rangy dependency. Is this worth the trouble it may cause / what are the main reasons to avoid using rangy. I was wondering what your reasons were for wanting to remove this dependency (is it mainly a performance thing?) I know rangy is used all over the place to help with selections in Arte (there's a whole folder of rangy-extensions) and am not really sure what the implications of removing it and doing this all ourselves may be. I believe we do some fairly complex operations with selections and am a little concerned about whether or not its worth it to try and port all of it. - For example - https://github.com/vistaprint/ArteJS/blob/master/src/editor/lib/rangy-extensions/rangy-extensions.js

[chriskwan]

To add to @gshenar's point, it might be worthwhile to build a range manipulation abstraction layer that will call rangy functions, so that anytime a component has to do any range operations it goes through this layer and does not have to call rangy directly. That way if we do decide to remove rangy, we only have to remove it in one place and the range manipulation API stays intact.

[chriskwan]

@leobalter, what's the difference of using .text() here vs using .html() in line 95?

[leobalter]

good catch, this must be html() indeed.