search

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 updates for increased efficiency #389

Closed regiskuckaertz closed 9 years ago

regiskuckaertz commented 9 years ago

Hi, As discussed in #388, I removed anything that was not strictly related to speeding up the execution of Scribe, primarily by removing TreeWalker which is slow as hell. I hope everything's in order for proper testing; I'm eager to help so please don't hesitate.

regiskuckaertz commented 9 years ago

oops, if I read it correctly, all failed tests seem to lead to a line-height and/or a misplaced BR. I'll check that out.

hmgibson23 commented 9 years ago

I'm happy with this. Be good to get a second set of eyes @rrees @cutandpastey

hmgibson23 commented 9 years ago

There's a few style issues. I won't go over them all but loosely, we use 2 spaces for basic indent and wrap all expressions with curly braces. So if etc. should always be accompanied by curly braces

cutandpastey commented 9 years ago

Sadly, I have a whole bunch of failing tests.

regiskuckaertz commented 9 years ago

@cutandpastey Hello, yes I should have done all the testing prior to doing the PR, but I had an error while running setup.sh with the certificate of the selenium domain name. As soon as I fix this, I'll be able to debug all my mistakes. I'll keep you posted asap. (Also, I've been profiling memory consumption and you should expect another PR with named function, it's much easier to debug :)

regiskuckaertz commented 9 years ago

@hmgibson23 Alright, I will go over all my changes and fix coding style issues. Yeah!

rrees commented 9 years ago

It's great that you've given so much love to the performance! It's been at the back of my mind for ages. If you can get the tests to pass then let's merge. If not do you want to try and break this PR into smaller parts so we can start merging the simpler parts.

regiskuckaertz commented 9 years ago

@rrees thanks a lot Robert, I appreciate. I have many ideas on how to improve the performance of Scribe. This is the first time I do such a huge PR, and it taught me that it's probably not the best way to help you, given how much work you have to do afterwards. Let's break the PR apart so that I can work on the failing tests while you can move forward, shall we? (I may need some guidance on how to do that with Git)

rrees commented 9 years ago

@regiskuckaertz The simplest way to do it is to use branches

Probably the easiest way to do this now is to run the following, which will reset your master to Scribe's master and save all the current changes you've got to :

git remote add upstream https://github.com/guardian/scribe.git
git branch all-changes
git fetch upstream
git reset --hard upstream/master

You can now branch and make a smaller change and submit that via the Github interface:

git checkout -b my-small-change
git push origin my-small-change
regiskuckaertz commented 9 years ago

Closing this one for good.