Skip to content
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

Refactor #27

Closed
8 tasks
madeleineostoja opened this issue Sep 1, 2016 · 9 comments
Closed
8 tasks

Refactor #27

madeleineostoja opened this issue Sep 1, 2016 · 9 comments

Comments

@madeleineostoja
Copy link

madeleineostoja commented Sep 1, 2016

#29 Simpla-text needs to be rebuilt from the ground up to polished and ready for 1.0. The biggest change will be replacing Scribe with a modern, abstracted richtext framework that doesn't rely on the native (broken) contenteditable API.

Major components:

  • Replace Scribe with abstracted framework (Draft, Slate, Quill) and abstract/split out to simpla-richtext-behavior.
  • Generalise sm-ui-toolbar into resusable singleton, not tightly bound to text
  • Remove placeholder element, use pure CSS solution instead (fixes Placeholder text goes off page if too long #26 and other issues)
  • Seperate editing/viewing logic (editor should not be loaded at all until/if edit mode)
  • Move to Simpla SDK (replace all utility components)
  • Tests
  • Rethink and consolidate interface (properties, methods). Make any breaking changes we need to.
  • Migrate to Polymer 2.0 or SkateJS
@madeleineostoja
Copy link
Author

Worth holding off on this until Polymer 2.0 is released mainline, and upgrade to that at the same time. Will update tasklist to reflect.

@madeleineostoja
Copy link
Author

madeleineostoja commented Sep 28, 2016

Looking into richtext frameworks a bit more, Draft and Slate are really our only long-term viable options. They're both built on React, which would almost make them untenable for the weight, but there's preact-compat 🎉 . They both should work with that (brings preact to 100% API compatibility), which would make the React(ish) dep absolutely fine.

@bedeoverend
Copy link
Contributor

bedeoverend commented Sep 28, 2016

@seaneking I had a play with Slate + preact-compat a little while back, it looks good. Unfortunately slate I'm pretty sure still injects a wrapper contenteditable div into the element. It isn't necessarily the end of the world, but it's a real pain. Note that we've been wrapping content during editing on Shady DOM browsers anyway, and so far hasn't been an issue.

Also re: size - I don't think we should stress too much about the size. Given we're only going to load in the functionality when Simpla becomes editable, it's ok if it takes a moment to load in those dependencies.

@bedeoverend
Copy link
Contributor

And wow, looks like Draft wraps up their content in 3 divs 😒

@madeleineostoja
Copy link
Author

madeleineostoja commented Sep 28, 2016

It's only while editing though, so as long as those elements don't have any styling associated to them it should be okay. The only edge case is if someone has relied on direct children for styling (ie: simpla-text > p), which will break during editing. We can document that edge case, and if that's our biggest problem with simpla-text, I'd be very happy.

That said we should def look into if we can somehow keep their wrappers in shadow dom and reflect content to light dom, even if we just clone the content and hide the shadow dom stuff, or something to that effect.

And re: size, are you sure we can only load in (as opposed to execute) a dep when editable? Even so, if we're not actually using React and just need it as a backing to a lib, then Preact would be awesome.

Also slightly off-topic, but I think we should make simpla-behaviors bower component, which contains a library of useful global utilities/behaviours/imports, and include a richtext behavior that wraps up one of these libs. Because there are going to be tonnes of elements that need richtext to some extent, and having them all run off the same engine (and dedupe it) would be really good (ie: <link rel="import" href=".../simpla-behaviors/richtext.html">)

Also FWIW if it comes down to it I'd much prefer to default to Draft, even if Slate has the cleaner API and such, because you just can't beat the fact that Draft has been battle-hardened on Facebook for years.

@bedeoverend
Copy link
Contributor

What frustrates me, all we need (in my limited understanding of react) is just that render function inside Slate to attach all those attributes / listeners etc. onto simpla-text.

As for the wrapper thing, unfortunately already tried that (though maybe try again with the new shadow DOM API) - essentially the contenteditable attribute element must be in the light DOM - contenteditable has no affect trying to pierce through the Shadow DOM boundary. so <div contenteditable><slot></slot></div> doesn't work. Anyway, worth investigating again.

@bedeoverend
Copy link
Contributor

bedeoverend commented Sep 28, 2016

I take it all back. Made a quick pen looks like it's working in the new v1, very very nice. That works on FF too using https://github.com/skatejs/named-slots polyfill. Still wouldn't work with Slate / Draft out of the box, but opens up a few doors.

Edit: doesn't appear to work on Safari. Doesn't work at all in the codepen, doing it manually in safari you can click on contenteditable, and it becomes focused, but can't change the content. Also further digger shows flaws - I'd say its a bug that it is working, reading the spec it implies it shouldn't work AFAIK

@bedeoverend
Copy link
Contributor

Just to clarify on the content editable and slot, it does work in some browsers, but it definitely is a bug and shouldn't happen.

@madeleineostoja
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants