Skip to content

Prevent GitHub adding a <p> tag to <td> inside issue comment. - #754

Merged
orta merged 3 commits into
danger:masterfrom
patrickkempff:feature/github-issue-template-layout
Nov 5, 2018
Merged

Prevent GitHub adding a <p> tag to <td> inside issue comment.#754
orta merged 3 commits into
danger:masterfrom
patrickkempff:feature/github-issue-template-layout

Conversation

@patrickkempff

Copy link
Copy Markdown
Contributor

Thanks for this awesome project. It looks awesome! There was one thing that caught my attention; the vertical alignment of content in the table seemed somewhat off.

Before:
screen shot 2018-11-05 at 12 44 58

After:
screen shot 2018-11-05 at 12 43 59

This PR fixes that.

After comparing the implementation and the code that GitHub renders. I found that the GitHub markdown implementation adds an

tag inside the when it thinks the text should be a text block (because of the linebreak before and after ${message}). When it appends a <p> the user-agent add the default margin at the bottom of the paragraph. (as an example; chrome adds margin-bottom: 16px;) This messes up the vertical alignment.

I removed the linebreaks and github andeverything seems to render everything correctly.

HTML output by github:

Before:
screen shot 2018-11-05 at 13 01 01

After:
screen shot 2018-11-05 at 13 03 22

Important: I also updated the githubIssueTemplates.test.ts.snap snapshot reflect the changes.

PS. @orta I am sure this is not the right way but I wanted to thank you and the artsy crew for the inspiring talks @facebookHQ in london last July.

@orta

orta commented Nov 5, 2018

Copy link
Copy Markdown
Member

Hrm, part of me feels like this is intentional (otherwise why would it be so weird?) - let's give it a shot anyway, and see what happens.

@orta
orta merged commit 3419b64 into danger:master Nov 5, 2018
@peril-staging

peril-staging Bot commented Nov 5, 2018

Copy link
Copy Markdown
Contributor

Thanks for the PR @patrickkempff.

The Danger org conform to the Moya Community Continuity Guidelines, which means
that we want to offer any contributor the ability to control their destiny.

So, we've sent you an org invite - thanks patrickkempff 🎉

Generated by 🚫 dangerJS

@patrickkempff

Copy link
Copy Markdown
Contributor Author

Hrm, part of me feels like this is intentional (otherwise why would it be so weird?) - let's give it a shot anyway, and see what happens.

After some searching around I found an commit from 2 years ago by @macklinu.
This discussion seems to be related: #240 (comment). I am not sure if it is still relevant

@orta

orta commented Nov 5, 2018

Copy link
Copy Markdown
Member

It probably is, I wonder if maybe we will need to do multi-line detection or something like that to make sure it handles inline html

@macklinu

macklinu commented Nov 5, 2018

Copy link
Copy Markdown
Member

I wonder if the <p> tag had something to do with Markdown formatting within a table cell? Something I noticed in the before/after of the PR screenshot:

image

This definitely looks cleaner without the extra <p> tag, so if there's a good way to support inline markdown with this too, I'm all for it. 👍

@patrickkempff

Copy link
Copy Markdown
Contributor Author

@orta maybe we should revert this PR until we have a better solution handling both cases. What do you think?

@orta

orta commented Nov 5, 2018

Copy link
Copy Markdown
Member

It's not shipped yet, so maybe we can look to see if the string includes a few particular chars and just include that?

@patrickkempff

Copy link
Copy Markdown
Contributor Author

Markdown regression is fixed in #777

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.

3 participants