Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Nov 1, 2017. It is now read-only.

Add the default monospace font stack for <code> elements#427

Conversation

jacobbednarz
Copy link
Contributor

Unfortunately<code> elements weren't getting the monospace font stack they deserved so they looked a little out of place. 💄

Turns
github-old
into
github-new

Unfortunately <code> elements weren't getting the monospace font stack theydeserved so they looked a little out of place. 💄
@tobiasahlin
Copy link
Contributor

@jacobbednarz Thanks! We should avoid another font stack definition though, could you look at makingthis selector less specific instead?

@jacobbednarz
Copy link
ContributorAuthor

@tobiasahlin Can definitely give it a whirl. :)

@jacobbednarz
Copy link
ContributorAuthor

@tobiasahlin Having a look into this, there isn't too much that can be done with that selector as it controls all the inline code styles (for lists, definition lists, etc) so it as about stripped back as I can see it continuing to work. Additionally, if I turn thep code into justcode there is apre definition further down that causes a double border on the nicely formatted code blocks as they are outputted as<pre><code>.

To tidying this up, I can remove the font stack from that selector you mentioned and into thecode block further down making it more generic which achieves the same thing as I set out to do as well as not adding another font stack.

.content .verseblock-content,.content .sectionbody .dlistdt,.contentp>tt,.contentdlcode,.contentulcode,pcode {-webkit-border-radius:3px;-moz-border-radius:3px;border-radius:3px;-moz-background-clip: padding;-webkit-background-clip: padding-box;background-clip: padding-box;border:1px solid#ccc;background-color:#f9f9f9;padding:0px3px;display: inline-block;}

and

code {white-space: nowrap;font:12px Monaco,"Courier New","DejaVu Sans Mono","Bitstream Vera Sans Mono",monospace;}

What are your thoughts on this one? Feel free to let me know if you have another way I could approach this one as I could be overlooking something here.

@jacobbednarz
Copy link
ContributorAuthor

Hey@tobiasahlin,
Have you had a chance to review this one? Just looking for some feedback on the approach you would prefer taken for this update?

@jacobbednarz
Copy link
ContributorAuthor

Haven't heard back around this one - do@pengwynn or@atmos mind taking a look and giving this one the yay or nay?

@tobiasahlin
Copy link
Contributor

@jacobbednarz So sorry for completely dropping this ball. My bad.

Where are you seeingcode elements that aren't wrapped in apre? That's where they should inherit the font-stack from, and if it's missing it, it could be a markup problem, rather than a CSS problem.

@jacobbednarz
Copy link
ContributorAuthor

@jacobbednarz So sorry for completely dropping this ball. My bad.

All good - I thought you may have been pulled onto another project and just left this little old pull request behind. 😉

Where are you seeing code elements that aren't wrapped in a pre?

Some examples are in tables (oauth/index.html#L105) and<p>'s (activity/events/types#L96)

@atmos
Copy link
Contributor

Screenshots help. 😉

@jacobbednarz
Copy link
ContributorAuthor

My apologies. :p Here is an example of one.

non-pre-code-blocks

@jacobbednarz
Copy link
ContributorAuthor

And the link -https://developer.github.com/v3/oauth/ to a page with some more examples.

@tobiasahlin
Copy link
Contributor

@jacobbednarz Nicely spotted! Looking at the current font stack, it's a bit arbitrary. Would you mind changing it to be identical to github.com? I.e.Consolas, 'Liberation Mono', Courier, monospace;. Both for thepreselector, thecode selector that you expanded, and the…, p code selector that you mentioned above.

With that in, I'd be happy to merge this:shipit:

- Make the <code> font stack more consistent from GitHub.com.- Refactor code/pre selector rules to be generic enough to catch allthe lingering code blocks.
@jacobbednarz
Copy link
ContributorAuthor

Thanks@tobiasahlin! Can you just check that commit10d0f78 is what you are after?

@@ -1017,7 +1017,6 @@ div.sidebar-module ul ul li span {
.content dl code,
.content ul code,
p code {
font: 12px Monaco,"Courier New","DejaVu Sans Mono","Bitstream Vera Sans Mono",monospace;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to keep the font stack here, or it would be dropped from.content .verseblock-content,.content .sectionbody .dlist dt, and.content p > tt.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I could be wrong but from what I can see thecode block onL#1157 further down catches it and applies the correct styling - I think that was why I chose to have a generic catch allcode selector.

pcode

contentulcode

Also, I wasn't able to find.content .verseblock-content or.content .sectionbody .dlist dt used anywhere in the markup so perhaps it has since been removed and we can safely remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jacobbednarz 💥 You're right!

@tobiasahlin
Copy link
Contributor

@jacobbednarz Looks great apart from my comment!

tobiasahlin added a commit that referenced this pull requestApr 8, 2014
…orrect-monospace-fontAdd the default monospace font stack for <code> elements
@tobiasahlintobiasahlin merged commit6dce70b intogithub:masterApr 8, 2014
@tobiasahlin
Copy link
Contributor

@jacobbednarz Thanks! ✨ 💎

a6eeece6-be16-11e3-9266-0a89b2a2d05d

@jacobbednarz
Copy link
ContributorAuthor

Awesome! Thanks for the feedback and help with getting this one out 🍰

hubot pushed a commit that referenced this pull requestJan 19, 2015
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@jacobbednarz@tobiasahlin@atmos

[8]ページ先頭

©2009-2025 Movatter.jp