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

Add page up and down key support #15

Merged
merged 1 commit into from Jun 26, 2013
Merged

Add page up and down key support #15

merged 1 commit into from Jun 26, 2013

Conversation

tzi
Copy link
Contributor

@tzi tzi commented May 20, 2013

Hi!

Bespoke.js is completly awesome.
I make HTML presentations for a long time, but I always needed to overwrite a lot of CSS style. That was before :)

So I made a presentation for a web conference in France.
The page down and up key support was missing, and that's what use the remote controllers.
I also added the Shift+Space key combination support, just because I like to use it.
I made some refactoring to improve code readability of this section.

I hope you will like it.

See you,
Thomas.

@markdalgleish
Copy link
Member

Thanks for the pull request!

I finally had a chance to have a proper look at this. Unfortunately your pull request pushes Bespoke.js over the 1KB (min'd and gzipped) limit. If you run grunt, you'll see the final size for bespoke.min.js.

Would you like to force push an update to your fork to get your pull request below 1KB, or would you prefer I do it myself? I like the page up/down changes since it works with presentation remotes, but I think you can leave the shift+space combo out for now since the bytes needed to achieve it are very precious :)

@tzi
Copy link
Contributor Author

tzi commented Jun 2, 2013

Thanks for your answer.

It's awesome to have such great objective as keep bespoke under 1KB!
I made my PR to improve readability. I will just remove this part to reach your goal.

I let you know when it's done.
Thomas.

@tzi
Copy link
Contributor Author

tzi commented Jun 12, 2013

Hi Mark!

I played with grunt and updated my pull request.

The current master branch is announced as 508 bytes (min+gzip). So I guess there was a lot of changes since my first PR.
My updated PR branch is marked as 523 bytes.

Is it OK for you?
Or should I try to gain this 15 bytes?

See you,
Thomas.

@markdalgleish
Copy link
Member

Sorry in advance for being a pain :)

I really want the page up and down support, and I'd love for you to get the credit for it, so could you please do the following?

  • reset your branch to match origin/master
  • add the code to handle page up/down, nothing else
  • run the build and make sure that the 'grunt-micro' step passes (grunt-micro uses the same Gzip function as microjs.com, unlike grunt-contrib-uglify)

Let me know if you have any issues :)

Cheers,
Mark

@tzi
Copy link
Contributor Author

tzi commented Jun 25, 2013

Hi Mark!

You're in your right to be a pain :P

I made the modifications in the right order, at least I think so.
I tried to reuse the syntax tricks that you used, but I had to change the condition statement to go below the 1kb target.

This PR has now a weight of 1018 bytes, instead of 1011 bytes for the current master.

Let me know if it's ok for you ;)

Cheers,
Thomas.

@markdalgleish
Copy link
Member

Thanks for that, it's looking pretty good. The only problem now is the indenting. Would you be able to modify your commits to use tabs instead of spaces and then force push to your branch? Bonus points if you squish your two commits into one ;)

@tzi
Copy link
Contributor Author

tzi commented Jun 26, 2013

I took the bonus point!
But, unless I'm mistaken, I see no space indentation.

Cheers,
Thomas.

markdalgleish added a commit that referenced this pull request Jun 26, 2013
Add page up and down key support
@markdalgleish markdalgleish merged commit c670964 into bespokejs:master Jun 26, 2013
@markdalgleish
Copy link
Member

Perfect! Thanks for the pull request :)

@tzi
Copy link
Contributor Author

tzi commented Jun 27, 2013

Yeah \o/

@tzi tzi deleted the pr__page-up-and-down branch June 27, 2013 08:20
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.

None yet

2 participants