-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feature/inline components #284
Conversation
…e renderspec interface
…ory attachment component
… the exported html
…nline-components-sample-plugin into feature/inline-components
This reverts commit 3aebd8e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall extremely impressive, well done
Some comments about the shape of this api, but it looks solid overall
if (componentSpec) { | ||
const component = new ModelInlineComponent(componentSpec, props, state); | ||
this.model.change( | ||
(mutator) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the command seems to break when selection is not collapsed
tested using the sample-plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved in 8177287
controller: Controller; | ||
} | ||
|
||
export default class InlineComponent< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though it's logical, I'd move this file out of the components subfolder, since this folder has quite some significance in the ember build system
I have my reservations about making this a class to extend from as well
There's not that much wrong with it, but I do think it slightly breaks ember's tradition of not using inheritance
Id maybe prefer that the ember-component would recieve some kind of control interface as arguments, like we do with the controllers
It feels less magic to me than just suddenly having some extra methods on this
but I don't think it makes a big difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified in: fbefa1b
…spec as additional argument
Thanks for the comments! I have modified most of the code to take into account the suggestions. |
I have also introduced some modifications to shiftedVisually so it takes into account inline components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now!
Adds support for the creation of inline interactive ember components which can be rendered inside the editor.