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

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

merged 6 commits into from Jun 27, 2018

Conversation

rdela
Copy link
Contributor

@rdela rdela commented Jun 8, 2018

Increase author link visibility and link avatar

  • www/src/components/blog-post-preview-item.js
  • www/src/templates/template-blog-post.js

Ref #3199 #4354 #5396

Confirmed addition of
transform: translateZ(0)
in blog-post-preview-item.js prevents image twitch in Chrome when hovering the card, way to go @fk

…iews

Increase author link visibility and link avatar
- `www/src/components/blog-post-preview-item.js`
- `www/src/templates/template-blog-post.js`

Ref #3199 #4354 #5396

Confirmed addition of
`transform: translateZ(0)`
in  blog-post-preview-item.js prevents image twitch in Chrome
when hovering the card, way to go @fk
@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 8, 2018

Deploy preview for using-drupal ready!

Built with commit 980aea5

https://deploy-preview-5813--using-drupal.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 8, 2018

Deploy preview for gatsbygram ready!

Built with commit 980aea5

https://deploy-preview-5813--gatsbygram.netlify.com

@rdela
Copy link
Contributor Author

rdela commented Jun 8, 2018

@KyleAMathews @fk this contains design changes if you wanna take a look

@pieh
Copy link
Contributor

pieh commented Jun 12, 2018

@fk
Copy link
Contributor

fk commented Jun 12, 2018

Hey @rdela! Thanks for sticking with us! 🤗
This is looking good <3, let's just def. get those 102% out—left comments.

@KyleAMathews
Copy link
Contributor

@fk doesn't look like your comments got posted?

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.

@fk
Copy link
Contributor

fk commented Jun 12, 2018

@KyleAMathews Argh, wrong textarea. :D Thank you!

@rdela
Copy link
Contributor Author

rdela commented Jun 15, 2018

Thanks @pieh @fk @KyleAMathews.

Looked like it was having build issues on Netlify at first after I pushed these commits,
9:52:54 PM: /usr/local/bin/build: line 32: gatsby: command not found
...but seems to have built after all:
https://deploy-preview-5813--next-gatsbyjs.netlify.com/blog/

Here is the screenshot comparison of font-weight: normal400[Edit 1] vs bold in blog post template I mention above in my comment on @fk's review:

normal-vs-bold-blog-post

Thinking same concern about taking away from the blog post title would apply here too and that we would want agreement between post and preview. I split preview and post changes into 2 commits so it would be easy to revert post one and just get rid of font-size: 102% if you were liking the bold in the post view @fk.

[Edit 1]: Top half of image and deploy preview author.ids are font-weight: 400 not normal in post template as I realized author.id h4 already had 400 set a few lines above when I went to remove font-size: 102% there. Works out the same I think but thought I would mention in case we want to specify exactly the same weight in both spots.

Side note: while I was puzzling over build issues I had some housekeeping ideas about maybe updating YARN_VERSION in netlify.toml from 1.2.1 to 1.7.0
https://github.com/gatsbyjs/gatsby/blob/master/netlify.toml#L3

On my blog I also have "--no-ignore-optional" in YARN_FLAGS which I got from
decaporg/gatsby-starter-decap-cms@b6cdfce

Setting RUBY_VERSION = "default"
https://github.com/rdela/rdela.com/blob/master/readme.md#ruby_version--default
definitely speeds up builds, as the log suggests:

9:49:48 PM: ** WARNING **
9:49:48 PM: Using custom ruby version 2.2.3, this will slow down the build.
9:49:48 PM: To ensure fast builds, set the RUBY_VERSION environment variable, or .ruby-version file, to an included ruby version.
9:49:48 PM: Included versions: 2.2.9 2.4.3 2.3.6

Probably worth opening a new issue with a title like Updating netlify.toml to speed up builds and giving everyone a chance to weigh in, but thought I would throw initial ideas out there while they are fresh in my mind in case anyone had preliminary thoughts or reasons why opening an issue was not a good idea.

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

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

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.

fk
fk previously approved these changes Jun 18, 2018
Copy link
Contributor

@fk fk left a comment

Choose a reason for hiding this comment

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

Lookin' good! thank you @rdela!

...which glamor offers to increase specifity
(ref. https://github.com/threepointone/glamor/blob/master/docs/selectors.md)
via @fk review
`src/components/blog-post-preview-item.js`
@rdela
Copy link
Contributor Author

rdela commented Jun 18, 2018

Left some comments in the (now outdated) review regarding specificity and the last commit: whether it should be reverted, or whether same concept (add && section) should be applied to template-blog-post.js. Lemme know what you think please @fk

@fk
Copy link
Contributor

fk commented Jun 19, 2018

👋 @rdela Just left a comment that should clarify things!

@rdela
Copy link
Contributor Author

rdela commented Jun 26, 2018

@fk Thanks, not sure I understand after looking through all that unless the goal is to remove !important.

From CSS Wizardry article:

We could fix this with an !important (line 26): jsfiddle.net/csswizardry/3N53n/1, but ewww, no thanks.

This still isn’t great, […]

Again, this is something of a hack, […]

Specificity-wise !important trumps the chained selectors according to the Spy vs Spy article you linked. Also, from the demo (emphasis mine):

7: A class selector just looks for a class on the element you're selecting. It doesn't do anything to exclude itself so it just stacks specificity. (Don't use this pattern.)

Also note the:

* not really

on Mathias’ slide.

All that said, whether or not you want to get rid of !important, feels like if we are relocating to && selector in blog-post-preview-item.js we should do the same in template-blog-post.js? See new comment in review.

@fk
Copy link
Contributor

fk commented Jun 26, 2018

the goal is to remove !important.

Yep!

All that said, whether or not you want to get rid of !important, feels like if we are relocating to && selector in blog-post-preview-item.js we should do the same in template-blog-post.js?

Yes; in this particular case I think we don't need either (see comment in code).

fk
fk previously approved these changes Jun 26, 2018
Conflicts:
www/src/templates/template-blog-post.js
L 137
`fixed={post.frontmatter.author.avatar.childImageSharp.fixed}`

* upstream/master: (651 commits)
  chore(release): Publish
  [www] Gatsby/rebeccapurple banner (#6161)
  delay page transition until we have all resources (#6158)
  Fix old query results showing up in develop (#6159)
  Tidy up the remark example (#6160)
  fix displaying of graphql resolver errors (#6142)
  Move store assignment out of component. (#6150)
  [v2] Fix doc not centered (#6146)
  chore(release): Publish
  [gatsby-source-contentful] pass host to client init (#6149)
  [v2] migrate gatsby-plugin-feed to use new Link (#6147)
  Make getElementById Conditional (#6145)
  chore(release): Publish
  update yarn.lock
  Update plugins.md (#6099)
  add gatsby-starter-skeleton (#6141)
  [gatsby-source-mongodb] added support for extra connection string params (#5972)
  create public page renderer and export correct version (dev / prod) depending on NODE_ENV (#5986)
  Treat integers longer than 32 bit as floats. (#6082)
  Export graphql TypeScript definition from gatsby package (#6096)
  ...
@rdela
Copy link
Contributor Author

rdela commented Jun 27, 2018

@fk Given #6076 / #6153 and that I already had a conflict that required updating figured I would target master with this one as well but I can build against v1 if that is easier

Ran Prettier and got rid of all use of !important in blog layouts in last commit

@rdela
Copy link
Contributor Author

rdela commented Jun 27, 2018

Copy link
Contributor

@fk fk left a comment

Choose a reason for hiding this comment

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

Nice! Thank you @rdela!

@m-allanson m-allanson merged commit 0e5aa1b into gatsbyjs:master Jun 27, 2018
@m-allanson
Copy link
Contributor

Thanks @rdela! This will go live on next.gatsbyjs.org for now, then gatsbyjs.org once v2 is released.

@rdela rdela deleted the author-link-styles branch November 16, 2018 07:07
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.

None yet

6 participants