Kindle ebook generation with working section navigation #4526

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
5 participants
@danchoi
Contributor

danchoi commented Jan 18, 2012

This commit corrects the Kindle ebook navigation to work on the Kindle Touch.

The commit adapts and incorporates the code from https://github.com/danchoi/docrails_kindle

NOTE: this commit introduces a dependency on nokogiri and kindlerb, but I don't
know the proper place outside this commit (which Gemfile or whatnot) to declare
this.

Daniel Choi
Generate Kindle ebook with working section navigation
NOTE: this commit introduces a dependency on nokogiri and kindlerb, but I don't
know the proper place outside this commit (which Gemfile or whatnot) to declare
this.

@ghost ghost assigned fxn Jan 18, 2012

@mipearson

This comment has been minimized.

Show comment Hide comment
@mipearson

mipearson Feb 21, 2012

Contributor

Oh nice. As the guy who did the original Kindle port, I was really hoping that somebody with a newer Kindle would jump on this.

+1

Contributor

mipearson commented Feb 21, 2012

Oh nice. As the guy who did the original Kindle port, I was really hoping that somebody with a newer Kindle would jump on this.

+1

@danchoi

This comment has been minimized.

Show comment Hide comment
@danchoi

danchoi Feb 21, 2012

Contributor

Thanks for doing the original port @mipearson

Contributor

danchoi commented Feb 21, 2012

Thanks for doing the original port @mipearson

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva May 1, 2012

Member

@danchoi your patch does not apply cleanly anymore, could you please bring it up-to-date with master, so it get more changes to get merged? Thanks.

/cc @fxn

@danchoi your patch does not apply cleanly anymore, could you please bring it up-to-date with master, so it get more changes to get merged? Thanks.

/cc @fxn

@danchoi

This comment has been minimized.

Show comment Hide comment
@danchoi

danchoi May 1, 2012

Contributor

Sorry, I think I messed up my fork of Rails. But the two commits above contain the whole patch.

Contributor

danchoi commented May 1, 2012

Sorry, I think I messed up my fork of Rails. But the two commits above contain the whole patch.

@danchoi

This comment has been minimized.

Show comment Hide comment
@danchoi

danchoi May 1, 2012

Contributor

I can try to submit a new patch if that's cleaner.

Contributor

danchoi commented May 1, 2012

I can try to submit a new patch if that's cleaner.

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik May 15, 2012

Member

Nope, certainly clean!

Member

steveklabnik commented May 15, 2012

Nope, certainly clean!

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Aug 21, 2012

Member

@fxn could you take a look on this? Let me know if I can help somehow, thanks.

@fxn could you take a look on this? Let me know if I can help somehow, thanks.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Aug 21, 2012

Member

@carlosantoniodasilva thanks for the ping.

Member

fxn commented Aug 21, 2012

@carlosantoniodasilva thanks for the ping.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Aug 21, 2012

Member

A few remarks:

  • I think it may be a little surprising that a library calls exec, is it needed or could we shell out with system?
  • The name DocrailsKindle is a little suspicious, docrails is not a project or anything. Could we have a different name for that one? Or could we merge both modules? Makes sense? Where is DocrailsKindle used?
  • Some commented out code remains, could you please clean that?
Member

fxn commented Aug 21, 2012

A few remarks:

  • I think it may be a little surprising that a library calls exec, is it needed or could we shell out with system?
  • The name DocrailsKindle is a little suspicious, docrails is not a project or anything. Could we have a different name for that one? Or could we merge both modules? Makes sense? Where is DocrailsKindle used?
  • Some commented out code remains, could you please clean that?
@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Aug 31, 2012

Member

@danchoi hey, any news over here?

Member

fxn commented Aug 31, 2012

@danchoi hey, any news over here?

@danchoi

This comment has been minimized.

Show comment Hide comment
@danchoi

danchoi Sep 4, 2012

Contributor

I'll try to make the fixes that you want this week @fxn. The changes you're asking for are certainly no big deal & I am happy to make them.

Contributor

danchoi commented Sep 4, 2012

I'll try to make the fixes that you want this week @fxn. The changes you're asking for are certainly no big deal & I am happy to make them.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Sep 4, 2012

Member

@danchoi thanks!

Member

fxn commented Sep 4, 2012

@danchoi thanks!

@danchoi

This comment has been minimized.

Show comment Hide comment
@danchoi

danchoi Sep 18, 2012

Contributor

Hi. I cleaned up the patches and redid them as a new patch.

danchoi/rails@cab0a6e

Contributor

danchoi commented Sep 18, 2012

Hi. I cleaned up the patches and redid them as a new patch.

danchoi/rails@cab0a6e

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Sep 19, 2012

Member

@danchoi if you add this patch to your master branch, github will update the pull request.

Member

steveklabnik commented Sep 19, 2012

@danchoi if you add this patch to your master branch, github will update the pull request.

@danchoi

This comment has been minimized.

Show comment Hide comment
@danchoi

danchoi Sep 19, 2012

Contributor

I added one more commit to clean up the bullet point formatting danchoi/rails@a93ca4c

I committed both of these to my master branch of my fork, but I'm not seeing them show up here.

I think I may have messed things up. Somehow my original fork of rails got deleted (maybe I did it, but I don't remember), and so I made a new fork today and created the these last two commits.

Contributor

danchoi commented Sep 19, 2012

I added one more commit to clean up the bullet point formatting danchoi/rails@a93ca4c

I committed both of these to my master branch of my fork, but I'm not seeing them show up here.

I think I may have messed things up. Somehow my original fork of rails got deleted (maybe I did it, but I don't remember), and so I made a new fork today and created the these last two commits.

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Sep 19, 2012

Member

Hm, how strange. @github must be acting a bit weird.

Member

steveklabnik commented Sep 19, 2012

Hm, how strange. @github must be acting a bit weird.

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Nov 17, 2012

Member

Hey @fxn, this merges cleanly; can we get it in?

Member

steveklabnik commented Nov 17, 2012

Hey @fxn, this merges cleanly; can we get it in?

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 17, 2012

Member

@steveklabnik cool thanks for the ping. I am out not, but I'll try to have a look later today.

Member

fxn commented Nov 17, 2012

@steveklabnik cool thanks for the ping. I am out not, but I'll try to have a look later today.

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Nov 17, 2012

Member

Is the file docrails_kindle.rb being used?

Is the file docrails_kindle.rb being used?

@danchoi

This comment has been minimized.

Show comment Hide comment
@danchoi

danchoi Nov 17, 2012

Contributor

I think I removed it.

Contributor

danchoi commented Nov 17, 2012

I think I removed it.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 17, 2012

Member

@danchoi as you say this PR does not seem to be sync'ed with the most recent changes. I think it would be better to review the real patch as is today. Would you mind opening a new PR with the last code?

Member

fxn commented Nov 17, 2012

@danchoi as you say this PR does not seem to be sync'ed with the most recent changes. I think it would be better to review the real patch as is today. Would you mind opening a new PR with the last code?

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 17, 2012

Member

Moved to #8254, closing here.

Member

fxn commented Nov 17, 2012

Moved to #8254, closing here.

@fxn fxn closed this Nov 17, 2012

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