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 IDs to the headers generated by RST #391

Merged
merged 3 commits into from
Feb 15, 2015
Merged

Add IDs to the headers generated by RST #391

merged 3 commits into from
Feb 15, 2015

Conversation

gjtorikian
Copy link
Contributor

Closes #71.

In #256, we stripped out the surrounding divs generated by RST for headers, because it padded too much whitespace and made documents look weird. Unfortunately, it looks like we also stripped out valuable id elements. I'm not sure how #71 predates the PR in #256, but in any event, we need to preserve the IDs.

This PR adds those IDs back as a simple a element. It's actually really tough to go back into the underlying <h_> tags, so this seems like a good compromise.

Note that I could not get the MiniTest suite to run on my local machine, so I changed a few parts about how it was called. I also found that the one and only RST test in the fixtures folder didn't accurately capture the problem described in #71, so I added a new file.

/cc @bkeepers

@bkeepers
Copy link
Contributor

bkeepers commented Nov 8, 2014

👍

@gjtorikian
Copy link
Contributor Author

Looks like the Minitest change broke Travis: https://travis-ci.org/github/markup/jobs/40349017

@bkeepers
Copy link
Contributor

@gjtorikian this good to go?

@gjtorikian
Copy link
Contributor Author

Yep, I tested against some of the repos in the original issue and this works fine. I'll open a separate PR to address the MiniTest issue.

gjtorikian added a commit that referenced this pull request Feb 15, 2015
Add IDs to the headers generated by RST
@gjtorikian gjtorikian merged commit ba1d568 into master Feb 15, 2015
@gjtorikian gjtorikian deleted the add-ids-to-rsts branch February 15, 2015 18:54
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.

.rst table of contents link do not work
2 participants