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

Kindle ebook generation with working section navigation #4526

Closed
wants to merge 5 commits into from
Closed

Kindle ebook generation with working section navigation #4526

wants to merge 5 commits into from

Conversation

danchoi
Copy link

@danchoi 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.

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
Copy link
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

@danchoi
Copy link
Author

danchoi commented Feb 21, 2012

Thanks for doing the original port @mipearson

@carlosantoniodasilva
Copy link
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
Copy link
Author

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
Copy link
Author

danchoi commented May 1, 2012

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

@steveklabnik
Copy link
Member

Nope, certainly clean!

@carlosantoniodasilva
Copy link
Member

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

@fxn
Copy link
Member

fxn commented Aug 21, 2012

@carlosantoniodasilva thanks for the ping.

@fxn
Copy link
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
Copy link
Member

fxn commented Aug 31, 2012

@danchoi hey, any news over here?

@danchoi
Copy link
Author

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
Copy link
Member

fxn commented Sep 4, 2012

@danchoi thanks!

@danchoi
Copy link
Author

danchoi commented Sep 18, 2012

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

danchoi@cab0a6e

@steveklabnik
Copy link
Member

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

@danchoi
Copy link
Author

danchoi commented Sep 19, 2012

I added one more commit to clean up the bullet point formatting danchoi@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
Copy link
Member

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

@steveklabnik
Copy link
Member

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

@fxn
Copy link
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
Copy link
Member

Is the file docrails_kindle.rb being used?

@danchoi
Copy link
Author

danchoi commented Nov 17, 2012

I think I removed it.

@fxn
Copy link
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
Copy link
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants