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 contrast for "reading time" info #1689

Merged
merged 2 commits into from Feb 18, 2019
Merged

Increase contrast for "reading time" info #1689

merged 2 commits into from Feb 18, 2019

Conversation

rhymes
Copy link
Contributor

@rhymes rhymes commented Jan 30, 2019

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

While perusing the Lighthouse report for dev.to I've noticed a "quick win" regarding the contrast between the white background and the text of the "reading time" info.

screenshot 2019-01-30 at 4 37 28 pm

Right now the background is #fff and the reading time text is gray which is rgb(128, 128, 128) which in turn results in this contrast level:

screenshot 2019-01-30 at 5 15 30 pm

I've darkened the text to rgb(89, 89, 89) with

color: darken($medium-gray, 5%);

instead of the current

color: lighten($medium-gray, 10%);

The contrast level now is:

screenshot 2019-01-30 at 5 16 21 pm

I have used this color contrast analyzer linked by Lighthouse.

ps. the diff includes some auto fixed spacing issues, the relevant line his here: https://github.com/thepracticaldev/dev.to/pull/1689/files#diff-8604f2aa24d368dba9c73900d331fbbbR575

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Before

screenshot 2019-01-30 at 5 20 14 pm

After

screenshot 2019-01-30 at 5 20 58 pm

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 30, 2019
@nickytonline
Copy link
Contributor

These are great PRs coming in @rhymes. I'm using lighthouse on my site for stuff like this as well. There's actually a beta Lighthouse CI that could be interesting to incorporate into the dev.to CI, https://github.com/ebidel/lighthousebot

@rhymes
Copy link
Contributor Author

rhymes commented Jan 30, 2019

Thanks @nickytonline - that bot seems interesting. I wonder how much it would slow down the build. I know @maestromac has been trying to make it faster for a while, adding another step might result in him trying to commit seppuku 🗡 😆

Anyhow, it's an interesting tool.

(ps. I think the build failure is unrelated to the CSS change)

@benhalpern
Copy link
Contributor

@rhymes sorry for letting this sit. It's good to be merged, but is now out of date, mind fixing the conflict and we'll merge it?

Google Lighthouse is flagging the low contrast this text has on its background
@rhymes
Copy link
Contributor Author

rhymes commented Feb 2, 2019

@benhalpern done :) You can re-check the diff that ignores whitespace changes: https://github.com/thepracticaldev/dev.to/pull/1689/files?utf8=%E2%9C%93&diff=split&w=1

Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

Looks good! Sorry for the delay.

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 15, 2019
@maestromac maestromac merged commit f8359d9 into forem:master Feb 18, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 18, 2019
@rhymes rhymes deleted the rhymes/better-reading-time-contrast branch February 18, 2019 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants