Card Unification RFC v2 #234
Conversation
| * [ ] Change card shape to object with `type`, `name`, `render` and optional `edit` properties | ||
| * [ ] Card's `type` is validated by renderer (dom renderer cannot render 'text', e.g.) | ||
| * [ ] Change arguments passed by editor to card's `render` (or `edit`) method | ||
| * [ ] single argument object with `name`, `buffer`, `isInEditor`, `options`, `editorEnv` and `payload` properties |
There was a problem hiding this comment.
isInEditor -> isEditing? "in editor" seems funky.
I'm kind of inclined to put it on env, but I'll let is go :-p Just don't want to make this a junk drawer.
editorEnv is actually env very specific to the card. The save etc. editorEnv definitely doesn't seem correct.
There was a problem hiding this comment.
I considered isEditing but to me it implies that the card is in "edit mode" (not "display mode"). isInEditor is intended to capture the fact that the card is being rendered for an editor (whether it is in edit or display mode at the moment). Same thing for editorEnv — that is an object of properties that could be useful when a card is being displayed/edited in an editor. editorEnv.save seems like it would be useful any time for a card in an editor (save(newPayload, false) is the way to update a card's payload without transitioning a card). editorEnv.edit is a no-op when the card is already in edit mode. It's redundant but doesn't seem confusing to me — I wouldn't expect someone to think that calling "edit" again while in edit mode would do something.
There was a problem hiding this comment.
Ah ok, I think I had the entire meaning of isInEditor wrong. Seems ok, I'm not sure what a better name would be 👍
editorEnv has some limited items like name for cards not rendered by the editor (source).
|
updated based on feedback |
| * [ ] Card's `type` is validated by renderer (dom renderer cannot render 'text', e.g.) | ||
| * [ ] Change arguments passed by editor to card's `render` (or `edit`) method | ||
| * [ ] single argument object with `name`, `onTeardown`, `isInEditor`, `options`, `editorEnv` and `payload` properties | ||
| * [ ] Return value of card's `render` (or `edit`) method is appended/concatenated by renderer |
There was a problem hiding this comment.
Looks like the other comments got removed here upon update. My response:
This doesn't make a huge difference either way, but the idea is to simplify things . Leave the rendering to the actual render, instead of doing it in the card.
- Saves writing
appendChild, concatenation,addSubviewcode in every card, and just let's the renderer do the right thing. appendChildabstractions can be kept in the renderer instead of re-implementing that in cards- The current pattern forces using wrapper dom element for cards.
- Passing in a buffer element opens easier possibilities of users leaking it
I don't understand the iOS argument. You return a UIView and the renderer does addSubview, instead of writing that line in every card.
There was a problem hiding this comment.
👍 Thanks for the feedback @gdub22. This makes sense, and I'm going to move forward with updating the renderers to work this way (using the return val as the rendered content and handling appending/concatting in the renderer itself)
|
implemented in #235 |
This is a continuation/distillation of the helpful feedback from #228.
do not merge
rendered
A few notes:
ember-mobiledoc-editororember-mobiledoc-dom-rendererwill not need to change much — the changes involved here are primarily for 1st-level consumers