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 anchors to markdown rendered headers #6219

Merged
merged 1 commit into from Feb 11, 2014

Conversation

5 participants
@cirosantilli
Contributor

cirosantilli commented Jan 31, 2014

Fixes ACCEPTING MERGE/PULL REQUESTS at http://feedback.gitlab.com/forums/176466-general/suggestions/5158863-markdown-headers-should-have-proper-id-s-and-popup

Screenshot:

screenshot from 2014-01-31 18 05 08

I'll finish implementation and write the tests once we agree on how the UI should look exactly (lets continue discussion there).

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Jan 31, 2014

Or do you prefer pure CSS Github clone mode (I'll squash this afterwards):

Before hover:

screenshot from 2014-01-31 21 32 28

During hover:

screenshot from 2014-01-31 21 32 44

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Feb 3, 2014

@cirosantilli I like github one. Can you fix tests so we can merge it?

@jbrooksuk

This comment has been minimized.

Contributor

jbrooksuk commented Feb 3, 2014

+1 for this.

I mentioned it somewhere in #5420, so it's good to see it getting some action.

@dosire

This comment has been minimized.

Member

dosire commented Feb 4, 2014

@cirosantilli Awesome that you are working on this.
@randx Thanks for commenting.

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 5, 2014

behaviours

By design choice, issue and MR comments' headers do not get IDs + links since:

  • they are often short
  • that could lead to id conflicts between multiple authors

All other markdown rendered headers should get IDs + links to those IDs:

  • READMEs
  • file views (blob + snippets)
  • issue, MR and milestone descriptions
  • the wiki
  • help pages that render markdown under doc/ such as doc/markdown/markdown.md rendered under app/views/help.

If I forgot to check one place where markdown is rendered, please tell me.

The change was explained on the markdown documentation section, in particular, how exactly IDs are generated from the header content. Please see the diff for intended behaviour.

screenshots

The major UI change is that on hover a header link image appears to the left of the header.

To do that, a bit of extra margin had to be added to the left of the markdown render container.

This is a sample from the issues:

screenshot from 2014-02-05 23 28 51

Note how comments do not get IDs by design choice:

screenshot from 2014-02-05 23 29 06

Except for that, the only other change was in help pages such as the markdown help, in which:

  • the text does not wrap anymore around the left toc.
  • rendered headers also have ids

I feel that this makes the entire help more uniform, and has the bonus that all header links will be on the same vertical line.

Also headers on the toc were converted into boldface to not generate ID conflicts:

screenshot from 2014-02-05 23 32 29

screenshot from 2014-02-05 23 32 48

A section was added explaining the header IDs, in particular exactly how the IDs are calculated from the text.

The end user must know that in order to avoid ID conflicts:

screenshot from 2014-02-06 01 28 51

doubts

  • On doc/markdown/markdown.md, before each header there is a <a name=""> tag before each header.

    I have renamed name to id since name is deprecated in HTML5, and removed the IDs that will dupe with the header names.

    Should I also remove the remaining anchors completely now that the headers have ids?

  • What should I do about help pages which are not rendered from markdown.

    They are sort of inconsistent with those that are because their headers don't have links, but there are not many of them.

    Should I:

    • convert those all that have headers to markdown
    • leave as is

implementation notes

Moved margins from .file-content.&.wiki to md-typesetting.

Removed .description extra div from .wiki on the issue, merge and milestone descriptions.

I have not added on hover / visibility tests they are not very portable on Capybara. If you feel that they are absolutely necessary, I can try to patch something up.

Moved markdown help page markdown tests from features/project/source/markdown_render.feature to features/help.feature since the help is not part of the projects.

@dosire

This comment has been minimized.

Member

dosire commented Feb 6, 2014

Thanks for all the hard work @cirosantilli !

@randx can you respond to his questions which I listed below?

  1. Should I also remove the remaining anchors completely now that the headers have ids? (I think yes)
  2. What should I do about help pages which are not rendered from markdown. (I think convert those all that have headers to markdown or even convert all to markdown so we can easily link to them when answering questions)
@Razer6

This comment has been minimized.

Member

Razer6 commented Feb 10, 2014

👍 This PR also fixes https://github.com/gitlabhq/gitlabhq/issues/6046 I guess

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Feb 11, 2014

@cirosantilli thank you for this PR. Few notes

To do that, a bit of extra margin had to be added to the left of the markdown render container.

remove them. text should be aligned same as header. Add negative margin to anchor icon instead

Should I also remove the remaining anchors completely now that the headers have ids?

yes

What should I do about help pages which are not rendered from markdown

leave as is. I dont like when Pull Request changes stuff not related to its title

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 11, 2014

@randx Sorry, I don't quite understand what you mean at "remove them. text should be aligned same as header. Add negative margin to anchor icon instead".

The header text is aligned same as the rest of the rest of the text as in the screen shots.

The margin was not added only to the headers, but to the entire preview container. If we want to have the links to the left like on the screenshot and GitHub, I don't see any other way around that, or else the link would appear above the container border.

Or do you want something different from the screenshot?

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Feb 11, 2014

@cirosantilli I see. In this case issue/MR title must have same margin. Ex. Issue subject has different margin in compare to description block

screen

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 11, 2014

@randx I agree. Same for the "Created by" line, right?

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Feb 11, 2014

@cirosantilli right! Thank you

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 11, 2014

Tried to address all issues raised:

  • remove the remaining anchors from doc/
  • add changelog line
  • align all issue-box text to match that of rendered markdown description:

screenshot from 2014-02-11 16 06 42

Rebased.

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 11, 2014

RSpec test have been failing on Travis CI due to 50min timeout on many unrelated branches I have pushed, and when they pass they are not far from 50min.

Locally RSpec tests pass in 20 min.

Is this not happening to other people also?

I am not sure what is the cause. Every time I checked new test pass dots . were being printed on the screen, so it does not seem that it is a single test that is taking a huge amount of time.

Maybe we just have to split RSpec tests into two batches as suggested in the Travis UI.

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Feb 11, 2014

@cirosantilli Yes we better split tests in future. So this PR is ready for merge?

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 11, 2014

All tests passed locally, and I tried to cover every point you guys mentioned, so I think... yes! =)

@dzaporozhets dzaporozhets merged commit 61748c9 into gitlabhq:master Feb 11, 2014

1 check failed

default The Travis CI build could not complete due to an error
Details
@dosire

This comment has been minimized.

Member

dosire commented Feb 11, 2014

Thanks @cirosantilli and @randx

As Mr. Readme I'm very happy we have linkable headers now, awesome!

@cirosantilli cirosantilli deleted the booktree:header-anchors branch Feb 11, 2014

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Feb 11, 2014

@dosire Markdown stuff is also important for my goal of GitLab as a textbook development tool, so I must thank you for merging!

@dosire

This comment has been minimized.

Member

dosire commented Feb 12, 2014

@cirosantilli Thanks for authoring.

@Razer6

This comment has been minimized.

Member

Razer6 commented Mar 23, 2014

@cirosantilli I just tried this, but unfortunately on GitLab 6.7 this looks a bit strange: https://gitlab.com/razer6/test-repo/blob/master/README.rdoc

You see this jumping anchor when hovering. Can you take a look?

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Mar 23, 2014

Definitely a bug. As far as you know:

  • does it happen only for rdoc files?
  • did rdoc files already have those on hover links when this was merged? I hadn't tested with rdoc =(

Also, on hover there are 2 character links:

  • one for the current header of type #label-header-converted-to-link. This is OK.
  • one fixed to #document, probably some RDoc built-in someone forgot to turn off.

My dev install is broken for now, if anyone reproduces on master lets open an issue and link here.

@Razer6

This comment has been minimized.

Member

Razer6 commented Mar 23, 2014

Only happens for rdoc files. Markdown works as expected. I don't know if there already was such a feature for rdoc. I created a new issue in https://github.com/gitlabhq/gitlabhq/issues/6599

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