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

Removing ::before ::after padding hack on markdown #360

Merged
merged 5 commits into from
Nov 8, 2017

Conversation

jonrohan
Copy link
Member

@jonrohan jonrohan commented Oct 5, 2017

Because of the ::before, ::after hack, the code blocks are not fully selectable. This pr removes the hack. It's been so long since this was introduced (~3 years ago) It's not fully clear what this was preventing. I think it has to do with scrollbars in Firefox and Android browsers.

I'd like to try and roll that back, then test on prod to see what bugs might crop up.

cc https://github.com/github/github/issues/58168

Copy link
Member

@broccolini broccolini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Down with testing this in github, don't want to approve until we've checked that doesn't break things though!

@@ -1,2 +1 @@
<link rel="stylesheet" href="https://unpkg.com/primer-css@9.2.0/build/build.css">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

padding: 0;
padding-top: 0.2em;
padding-bottom: 0.2em;
padding: 0.2em 0.4em;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One day in the future we should bring this into our proposed em-based scale and stick to common fractions.

letter-spacing: -0.2em; // this creates padding
content: "\00a0";
}
border-radius: 3px;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the $border-radius variable 😝

@@ -0,0 +1,18 @@
import React from 'react'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Yay for adding more stories! It would be great to add the rest of the markdown styles when you get a chance.

@jonrohan
Copy link
Member Author

jonrohan commented Oct 6, 2017

This was deployed to review-lab https://github.com/github/github/pull/79702 and I didn't find any regressions.

@jonrohan jonrohan added the v10 label Oct 27, 2017
@broccolini broccolini mentioned this pull request Nov 6, 2017
28 tasks
@jonrohan jonrohan changed the base branch from dev to release-10.0.0 November 8, 2017 19:08
Copy link
Member

@broccolini broccolini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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

Successfully merging this pull request may close these issues.

2 participants