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

Added Ability to select new text and pause. Also added a bookmarklet.html page. #63

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

smorin
Copy link

@smorin smorin commented Mar 9, 2014

I have made code changes to allow OpenSpritz to Pause the current Spritz. Also you can now select new txt and continue reading a new selection.

For a pull request the annoying thing is that I have references to my github account that need to be changed back to yours.

@smorin
Copy link
Author

smorin commented Mar 9, 2014

Additional comment I was trying to get keybindings to work with the jQuery hotkeys plugin. I didn't have time to figure out what was wrong. Hope these feature make it into your plugin.

@carrollgt91
Copy link

The pause feature is crucial. Excited to see this get added to the codebase.

@elicwhite
Copy link
Contributor

@smorin, thanks for doing all this work, it looks awesome and I'm glad you are so interested in this project. My guess though is that this pull request won't be merged by @Miserlou

A couple of housekeeping issues holding it back: please make sure pull requests are against the dev branch. Make sure you squash your commits so that it doesn't add a ton of commits to the main repo. Most importantly, pull requests should be as small in size as possible and only changing one thing in each. In this case, you are adding pause (which another pull request added), continuing on a new selection, key bindings, and added a ton of logic in spritzify_go(). Also, this is using the old jQuery version, which the dev branch (and everything moving forward) doesn't use.

The reason this should be many different pull requests is so that each feature can be reviewed separately. For example, just because another pull request added pause, this won't ever be merged. However if your pause was a separate pull request from everything else, we could ignore it and look at the other ones.

While it might seem like I am trying to shoot down your work, I am actually really truly glad you care enough about this project to do as much as you did. I want to see you be successful and get your changes merged which is why I took the time to write out this comment.

@smorin
Copy link
Author

smorin commented Mar 11, 2014

No problem.

All your comments make sense.

The truth of the matter I was in a rush have don't have time to fix it.
The changes are there so should be easy for someone to take them and
format them to roll the changes in.

I am currently running off my fork of the project.

On Tue, Mar 11, 2014 at 4:50 PM, Eli White [email protected] wrote:

Smorin, thanks for doing all this work, it looks awesome and I'm glad you
are so interested in this project. My guess though is that this pull
request won't be merged by @Miserlou https://github.com/Miserlou

A couple of housekeeping issues holding it back: please make sure pull
requests are against the dev branch. Make sure you squash your commits so
that it doesn't add a ton of commits to the main repo. Most importantly,
pull requests should be as small in size as possible and only changing one
thing in each. In this case, you are adding pause (which another pull
request added), continuing on a new selection, key bindings, and added a
ton of logic in spritzify_go(). Also, this is using the old jQuery version,
which the dev branch (and everything moving forward) doesn't use.

The reason this should be many different pull requests is so that each
feature can be reviewed separately. For example, just because another pull
request added pause, this won't ever be merged. However if your pause was a
separate pull request from everything else, we could ignore it and look at
the other ones.

While it might seem like I am trying to shoot down your work, I am
actually really truly glad you care enough about this project to do as much
as you did. I want to see you be successful and get your changes merged
which is why I took the time to write out this comment.

Reply to this email directly or view it on GitHubhttps://github.com//pull/63#issuecomment-37360612
.

Steve Morin | Hacker, Entrepreneur, Startup Advisor
twitter.com/SteveMorin | stevemorin.com
Live the dream start a startup. Make the world ... a better place.

@weidenrinde
Copy link

It seems, the pause is included, but the selection of text is not, right? I would find this a very valuable extension!

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

Successfully merging this pull request may close these issues.

4 participants