guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

Some contributions to Scribe #388

Closed regiskuckaertz closed 9 years ago

regiskuckaertz commented 9 years ago

Hello, a few weeks ago I started using Scribe for an internal project here at the Government of Luxembourg. It's a really great piece of software with an already comprehensive feature set, which is quite impressive given its short lifespan so far. As I was getting acquainted with how the code's organized, I thought about a few improvements (mostly moving things around) and committed them to my own fork. Here they are, for the most part:

I still have to do exhaustive tests, and there are areas I need to wrap my head around (pushHistory in particular), but in the meantime if you find anything worth adding to the master repo, please tell me and I'll be happy to contribute—I'm not a GitHub pro, so I might need some guidance here.

I also think Immutable could be used in many other places, so I will keep working on that, but as most of the code relies on vars and procedural code, I'm thinking of using transducers instead—I will definitely have look at that as well and keep you posted.

hmgibson23 commented 9 years ago

Hi, Glad that you've been having success using scribe. I think the least controversial and perhaps most helpful to us would the speed improvement stuff (using the TreeWalker everywhere and the Node.compareDocumentPosition stuff). Removal of temporary variables sounds good too.

I'd like to see lodash go, as personally I have my doubts about it's effectiveness for this type of project but other contributors might have different opinions.

If you want to make the pull request with just the stuff I mentioned I'll be happy to review it and get it in the repo.

Thanks!

regiskuckaertz commented 9 years ago

@hmgibson23 awesome! I will isolate all these things and make a PR asap.

JeSuis commented 9 years ago

:clap:

regiskuckaertz commented 9 years ago

Closing this issue as we've moved on with the PRs.