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

feat(color-contrast): add px unit to error messages #1634

Merged
merged 2 commits into from
Jun 19, 2019
Merged

Conversation

straker
Copy link
Contributor

@straker straker commented Jun 14, 2019

Add the size in px to the color contrast error message to avoid confusion with the pt unit.

"Fix any of the following:
  Element has insufficient color contrast of 3.87 (foreground color: #3d7e9a, background color: #eeeeee, font size: 12.0pt / 16px, font weight: normal). Expected contrast ratio of 4.5:1"

Linked issue: #1628

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: @dylanb

@straker straker requested a review from a team as a code owner June 14, 2019 16:54
dylanb
dylanb previously requested changes Jun 17, 2019
Copy link
Contributor

@dylanb dylanb left a comment

Choose a reason for hiding this comment

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

The commit should be marked as feat(color-contrast): because this is a new feature

jeeyyy
jeeyyy previously approved these changes Jun 17, 2019
Copy link
Contributor

@jeeyyy jeeyyy left a comment

Choose a reason for hiding this comment

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

LGTM!

In my opinion this could read better as ${((fontSize * 72) / 96).toFixed(1)}pt (${fontSize}px)

Eg: 13.5pt (18px)

But that may be a personal preference.

@straker
Copy link
Contributor Author

straker commented Jun 18, 2019

@JKODU I did that first, but since we were already in an parenthesis group I didn't know if that would be OK

(foreground color: #3d7e9a, background color: #eeeeee, font size: 12.0pt (16px), font weight: normal)

@stephenmathieson stephenmathieson changed the title fix(color-contrast): add px unit to error messages feat(color-contrast): add px unit to error messages Jun 18, 2019
@jeeyyy
Copy link
Contributor

jeeyyy commented Jun 18, 2019

@straker, it is logical to express font as say 300 italic 1.3em/1.7em serif, which is why I thought parenthesis may be better. But this is a nitpick. Happy with what you decide.

@straker straker dismissed stale reviews from stephenmathieson and jeeyyy via 91ca9f6 June 18, 2019 21:27
@straker straker merged commit 1712e46 into develop Jun 19, 2019
@straker straker deleted the colorContrastPx branch June 19, 2019 14:31
WilcoFiers pushed a commit that referenced this pull request Jul 22, 2019
* fix(color-contrast): add px unit to error messages

* change to show px in parenthesis
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.

4 participants