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.

Sign up for GitHub

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

Jump to bottom

Increase author link visibility & link avatars in blog posts + previews #5813

Merged
merged 6 commits into from
Jun 27, 2018
Merged

Increase author link visibility & link avatars in blog posts + previews #5813

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
37f7bab
Increase author link visibility and link avatars in blog posts + prev…
rdela Jun 8, 2018
3aad32e
lose font-size increase and bold
rdela Jun 15, 2018
b9c097e
lose font-size increase and bold for post too
rdela Jun 15, 2018
c495ea7
move keys that require !important to the && selector
rdela Jun 18, 2018
8508952
Merge remote-tracking branch 'upstream/master' into author-link-styles
rdela Jun 27, 2018
980aea5
remove !important from blog preview and post layout
rdela Jun 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 20 additions & 7 deletions www/src/components/blog-post-preview-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,21 @@ class BlogPostPreviewItem extends React.Component {
marginBottom: rhythm(2),
}}
>
<Img
<Link
to={post.frontmatter.author.fields.slug}
css={{
boxShadow: `none !important`,
borderBottom: `0 !important`,
Copy link
Contributor

Choose a reason for hiding this comment

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

You could move the keys (here and also the ones below) that require !important to the && selector which glamor offers to increase specifity (ref. https://github.com/threepointone/glamor/blob/master/docs/selectors.md).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like I should do the same here then?
https://github.com/gatsbyjs/gatsby/pull/5813/files/b9c097e3eba19af0a824098aefe1e812ed00125b#diff-3f9ee2e9c0e318b6189a8f786092c9f5R164

template-blog-post.js

L164

color: `${colors.gatsby} !important`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and committed a change with suggestion above. Changed my mind about the last comment though, and maybe the commit. Color me puzzled. Perhaps I am not understanding Glamor but I do not see how given HTML like

<a class="css-9rkwk9" href="/contributors/michelle-barker/">Michelle Barker</a>

the resulting CSS:

.css-9rkwk9.css-9rkwk9, [data-css-9rkwk9][data-css-9rkwk9] {
	color: #663399 !important;
	font-weight: normal;
}

is more specific than

.css-9rkwk9,[data-css-9rkwk9]{
	position:relative;
	z-index:1;
}

.css-9rkwk9.css-9rkwk9 seems redundant to me. If it is I can revert the last commit. If it makes sense to you then this is ready unless you want me to wrap

color: `${colors.gatsby} !important`,

in an && section in template-blog-post.js as well per above.

Copy link
Contributor

@fk fk Jun 19, 2018 •
edited

Choose a reason for hiding this comment

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

The chained selectors (e.g. .css-9rkwk9.css-9rkwk9) are what is actually doing the trick. To quote the genius named Harry Roberts (@csswizardry):

[…] you can chain a selector with itself to increase its specificity.

(via https://csswizardry.com/2014/07/hacks-for-dealing-with-specificity/#safely-increasing-specificity)

Also see

Copy link
Contributor Author

@rdela rdela Jun 26, 2018 •
edited

Choose a reason for hiding this comment

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

So what about previous comment then?

Seems like I should do the same here then?
https://github.com/gatsbyjs/gatsby/pull/5813/files/b9c097e3eba19af0a824098aefe1e812ed00125b#diff-3f9ee2e9c0e318b6189a8f786092c9f5R164

template-blog-post.js

L164

(add && selector to template-blog-post.js and move color: `${colors.gatsby} !important`, to it?)

Copy link
Contributor

Choose a reason for hiding this comment

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

L164 seems to work fine without the !important:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally we should avoid (and have avoided—I'm quite sure I'm guilty here as I wasn't aware of && when I first started using Glamor ;-)) using !important(see e.g. https://developer.mozilla.org/en-US/docs/Web/CSS/Specificity#The_!important_exception).

If you need to override styles try if Glamor's && will do and make !important a last resort.

position: `relative`,
zIndex: 1,
"&&": {
fontWeight: `normal`,
":hover": {
background: `transparent`,
},
},
}}
><Img
alt=""
resolutions={avatar}
css={{
Expand All @@ -37,7 +51,7 @@ class BlogPostPreviewItem extends React.Component {
// prevents image twitch in Chrome when hovering the card
transform: `translateZ(0)`,
}}
/>
/></Link>
<div
css={{
display: `inline-block`,
Expand All @@ -56,15 +70,14 @@ class BlogPostPreviewItem extends React.Component {
<Link
to={post.frontmatter.author.fields.slug}
css={{
boxShadow: `none !important`,
borderBottom: `0 !important`,
color: `${colors.gatsby} !important`,
fontSize: `102%`,
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to increase font-size here because we don't mix Futura with Spectral.

position: `relative`,
zIndex: 1,
"&&": {
fontWeight: `normal`,
fontWeight: `bold`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try normal here to give the link less weight—we don't want this to take away from the blog post title too much:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mirrored these requested changes to the preview on post as well, thinking we would want agreement, but I split them into 2 commits so it would be easy to revert if you liked the bold in the post view. I will add a screenshot comparison in a sec.

":hover": {
color: colors.gatsby,
background: `transparent`,
background: colors.ui.bright,
},
},
}}
Expand Down
19 changes: 16 additions & 3 deletions www/src/templates/template-blog-post.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ class BlogPostTemplate extends React.Component {
flex: `0 0 auto`,
}}
>
<Img
<Link to={post.frontmatter.author.fields.slug}><Img
resolutions={
post.frontmatter.author.avatar.childImageSharp.resolutions
}
Expand All @@ -147,7 +147,7 @@ class BlogPostTemplate extends React.Component {
display: `inline-block`,
verticalAlign: `middle`,
}}
/>
/></Link>
</div>
<div
css={{
Expand All @@ -161,9 +161,22 @@ class BlogPostTemplate extends React.Component {
...scale(0),
fontWeight: 400,
margin: 0,
color: `${colors.gatsby} !important`,
fontSize: `102%`,
fontWeight: `bold`,
}}
>
{post.frontmatter.author.id}
<span css={{
borderBottom: `1px solid ${colors.ui.bright}`,
boxShadow: `inset 0 -2px 0 0 ${colors.ui.bright}`,
transition: `all ${presets.animation.speedFast} ${
presets.animation.curveDefault
}`,
"&:hover": {
background: colors.ui.bright,
},
}}
>{post.frontmatter.author.id}</span>
</h4>
</Link>
<BioLine>{post.frontmatter.author.bio}</BioLine>
Expand Down