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

Adds margin to code highlights. Tweaks colorscheme of notes. #1089

Merged
merged 2 commits into from May 14, 2013

Conversation

Projects
None yet
4 participants
@zachgersh
Contributor

zachgersh commented May 12, 2013

Sorry @cobyism if this is terrible :) Tweaks #1076

@parkr

This comment has been minimized.

Member

parkr commented May 12, 2013

Got some screenshots? :)

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented May 12, 2013

screenshot is attached to the main bug where Cobyism attached his screens.

@parkr

View changes

site/css/style.css Outdated
}
}
.highlight, p > pre, p > code, p > nobr > code, li > code {
background: #333;

This comment has been minimized.

@parkr

parkr May 12, 2013

Member

i'd keep the #333 i think

@cobyism

This comment has been minimized.

Member

cobyism commented May 13, 2013

Screenshot from the other thread, for reference:

Yep, font-size and padding/margin are a definite improvement. As for the actual color of the code element though, I think this needs to be tailored a bit more to each of the different note types (i.e. dark red/blue/brown) instead of just being an unsaturated grey. It might be possible to do with this a partial transparency (i.e. background: rgba(0,0,0,.5);) so that some of the hue from the underlying element shows through, or maybe it will have to be given three different values. Does that make sense?

@cobyism

View changes

site/css/style.css Outdated
@@ -515,17 +515,19 @@ pre, code {
@media (min-width: 768px){
pre, code {
font-size: 16px;
font-size: 0.75em;
}
}
.highlight, p > pre, p > code, p > nobr > code, li > code {

This comment has been minimized.

@cobyism

cobyism May 13, 2013

Member

Am I right in understanding that the diff here affects all code elements in the site? If so, I’d have a strong preference for leaving the look of code elements contained in the normal article sections the same—instead, I think what might be needed here is to override some of what is being applied to these elements with a nested CSS selector, so it only targets code elements contained within .note elements.

This comment has been minimized.

@parkr

parkr May 13, 2013

Member

👍 ^^

This comment has been minimized.

@zachgersh

zachgersh May 13, 2013

Contributor

Yes, was a global change to padding and color which I can pull back and make directed at just notes.

I think the spacing / size would be good to apply to the paragraph code elements as well. Would you have a quick glance at that cobyism?

On May 13, 2013, at 7:14 AM, Parker Moore notifications@github.com wrote:

In site/css/style.css:

@@ -515,17 +515,19 @@ pre, code {

@media (min-width: 768px){
pre, code {

  • font-size: 16px;
  • font-size: 0.75em;
    }
    }

.highlight, p > pre, p > code, p > nobr > code, li > code {
^^


Reply to this email directly or view it on GitHub.

This comment has been minimized.

@cobyism

cobyism May 13, 2013

Member

I feel like the styles for code elements in paragraphs read pretty well already, so should be fine as they are:

screen shot 2013-05-13 at 3 37 49 pm
screen shot 2013-05-13 at 3 38 02 pm

To my 👀, they’re already a reasonably appropriate font-size, and have enough of a margin between the word boundaries. The real issue here I think is confined to the code elements within the .note containers, where normal text is scaled down and colored appropriately, but code elements got left out of the equation in the initial design I did.

This comment has been minimized.

@parkr

parkr May 13, 2013

Member

If, for each note, the code was shaded with the color of the note (gold, blue, red) that would be cool. But I agree it's the font size and awkward proportions of the padding around the code in the notes is what we're trying to tackle. So just shrink things down and blend them in a bit.

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented May 13, 2013

One more bit I wanted to point out which I think will be addressed by the size change:

screen shot 2013-05-13 at 8 15 29 am

Did you want to go back to black @cobyism or did you like the grey with a bit of color showing through?

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented May 13, 2013

As suggested added partial transparency and minimized the scope of where we change the size / margin of the code elements

screen shot 2013-05-13 at 8 46 59 am

@cobyism

This comment has been minimized.

Member

cobyism commented May 13, 2013

@zachgersh I still think the contrast is way too much. Ideally we want this to be reasonably subtle, so that it looks like it’s just recessed a little bit into the blue flag/ribbon section. Here’s a quick stab at tweaking things just in webinspector to show you what I mean:

screen shot 2013-05-13 at 4 55 08 pm

Also, I noticed that in your latest screenshot, the <code> element in the header of that .note (where it says "The --config explicitly specifies…") doesn’t have rounded corners 😦 That’s clearly an oversight of mine I didn’t think about in the original design, sorry about that! I’m sure it will just be a case of fixing a rule somewhere though so it’s taken into account—so if you could work out a way to do that too, that would be

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented May 13, 2013

Thanks @cobyism that is perfect and something I can duplicate tonight.

Yeah, I noticed the title as well and thought I had botched something but I will add a case for that as well.

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented May 14, 2013

screen shot 2013-05-13 at 11 34 29 pm

@cobyism

View changes

site/css/style.css Outdated
@@ -519,7 +519,7 @@ pre, code {
}
}
.highlight, p > pre, p > code, p > nobr > code, li > code {
.highlight, p > pre, p > code, p > nobr > code, li > code, h5 > code {

This comment has been minimized.

@cobyism

cobyism May 14, 2013

Member

This definitely works, however given in the .note code rule below you’re redefining essentially the exact same set of box-shadows, I’d be tempted to change the h5 > code targeted here so that it’s just .note code at the end of the list instead—and then keep the next CSS rule definition below as an override for .note code. That way you can 💀 the box shadow definition in the following section and just override the background, margins, and font-size instead. Not sure if I explained that very well 😕

@cobyism

This comment has been minimized.

Member

cobyism commented May 14, 2013

@zachgersh 👍 That looks 🔥. Left a small note about a small way to possibly refactor it to reduce some repetition, but other than that I say :shipit:

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented May 14, 2013

hey @cobyism explained it fine. I actually had to leave the h5 bit so that we make sure to apply the same style to the title. I did what you said and dropped the box shadow in my override and then just added note to the end of the chain.

Looks good to me?

@parkr!

@parkr

This comment has been minimized.

Member

parkr commented May 14, 2013

Looks baller!

parkr added a commit that referenced this pull request May 14, 2013

Merge pull request #1089 from zachgersh/cobyism_style
Adds margin to code highlights. Tweaks colorscheme of notes.

@parkr parkr merged commit a85f7c1 into jekyll:master May 14, 2013

1 check passed

default The Travis CI build passed
Details

parkr added a commit that referenced this pull request May 14, 2013

@cobyism

This comment has been minimized.

Member

cobyism commented May 15, 2013

🐧 👍

@parkr

This comment has been minimized.

Member

parkr commented May 15, 2013

Pushed up to the site btw - looks great :) Nice work!

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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