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

Add Diffstat component #950

Merged
merged 7 commits into from
Oct 25, 2019
Merged

Add Diffstat component #950

merged 7 commits into from
Oct 25, 2019

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Oct 21, 2019

This adds the .diffstat component to Primer CSS.

Docs Preview

Simple With more details
Screen Shot 2019-10-21 at 1 37 27 PM Screen Shot 2019-10-21 at 1 37 44 PM

TODO

  • Add styles from dotcom
  • Add documentation
  • Maybe tweak colors to not use darken()

TODO on dotcom

/cc @primer/ds-core

@vercel
Copy link

vercel bot commented Oct 21, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/primer/primer-css/azbv71e7c
🌍 Preview: https://primer-css-git-diffstat.primer.now.sh

@simurai
Copy link
Contributor Author

simurai commented Oct 21, 2019

Maybe tweak colors to not use darken()

Here how it would look if we replaced darken($green-400, 5%);.

$green-400 darken($green-400, 5%); $green-500
400 450 500
hsl(134, 62%, 51%) hsl(134, 62%, 46%) hsl(134, 61%, 41%)
#34d058 #2cbe4e #28a745

Hmm.. $green-500 would have more contrast compared to the background, but $green-400 would have more contrast compared to the red blocks. Like it's easier to see the difference between green/red with $green-400.

Edit: At our meeting we decided to keep darken($green-400, 5%); for now. At some point we can consider adding a new variable like .green-diff or similar.

@shawnbot shawnbot changed the base branch from master to release-13.2.0 October 22, 2019 18:52
@shawnbot
Copy link
Contributor

@simurai if we name the variable for this $bg-<anything>, then it'll pass the new stylelint rules because they just want any variable that begins with bg-.

Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

My suggestion is to create two new variables for these colors: $bg-diffstat-added: darken($green-400, 5%) and $bg-diffstat-deleted: $red-600. Currently, if we were to run this through our new stylelint rules we'd have primer/colors violations for both values, since neither is a $bg- variable.

src/labels/diffstat.scss Outdated Show resolved Hide resolved
@shawnbot shawnbot mentioned this pull request Oct 22, 2019
15 tasks
Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

@simurai FYI I moved the new $bg-diffstat-* variable declarations to colors.scss and flagged them with !default per existing conventions. I think this is good to go!

@simurai
Copy link
Contributor Author

simurai commented Oct 25, 2019

FYI I moved the new $bg-diffstat-* variable declarations to colors.scss and flagged them with !default per existing conventions.

Was wondering if we should organize variables per component or property. 👍 Good for now.

@simurai simurai merged commit d0ae4a6 into release-13.2.0 Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants