-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use more descriptive CSS variable names (#135) #147
Conversation
Signed-off-by: Nathan Murthy <drip.dr0p.dr0p@gmail.com>
Hey @natemurthy thanks for this PR 🎉 I think the intention of the ticket was to decouple the names of the colors from their use in various components. So for example,
This would make things more readable, for example in line-graph.css, we currently have:
This references a latency color, but this "flash" has nothing to do with latency. It would perhaps be less confusing if it were to use:
|
Signed-off-by: Nathan Murthy <drip.dr0p.dr0p@gmail.com>
@rmars Have a look at b072a55. Are there other var names you'd want to change? |
hey @natemurthy thanks for the speedy updates! Ah yes! Forgot to mention in my comment above, I updated the ticket (#135) with a color tool we could use (http://chir.ag/projects/name-that-color/#56CCF2). I think ideally all the color variables in the taking a look at b072a55 now! |
Signed-off-by: Nathan Murthy <drip.dr0p.dr0p@gmail.com>
web/app/css/styles.css
Outdated
--warmgrey: #f6f6f6; | ||
--coldgrey: #c9c9c9; | ||
--latency-p99: var(--messageblue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave the --latency-pXX
vars defined since we have multiple places that will want to be referencing the same color (for example the latency p50 summary and the latency p50 chart line should be the same color, and I think it's fine for that to be named --latency-p50
), but the base variables can reference the new named blues:
--curiousblue: #2D9CDB;
--royalblue: #2F80ED;
--pictonblue: #56CCF2;
--latency-p99: var(--royalblue);
--latency-p95: var(--curiousblue);
--latency-p50: var(--pictonblue);
Then in line-graph.css we can use fill: var(--pictonblue);
in line 26 (as you've done!) but leave lines 13-22 to reference the original --latency-pXX variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, thanks for taking on this cleanup!
we should make sure all references to the renamed colors reflect the new name.
web/app/css/styles.css
Outdated
@@ -2,18 +2,17 @@ | |||
|
|||
:root { | |||
--white: #fff; | |||
--buttonblue: #4076c4; | |||
--messageblue: #2F80ED; | |||
--indigoblue: #4076c4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it appears we no longer use this blue anywhere. I'd be in favor of removing it.
web/app/css/styles.css
Outdated
--buttonblue: #4076c4; | ||
--messageblue: #2F80ED; | ||
--indigoblue: #4076c4; | ||
--royalblue: #2F80ED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some more references to messageblue
in this file as well as service-mesh.css
that should be updated to this new color
--latency-p99: var(--messageblue); | ||
--latency-p95: #2D9CDB; | ||
--latency-p50: #56CCF2; | ||
--subheader-grey: #828282; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some more references to --subheader-grey
in the codebase that should also be updated
--latency-p95: #2D9CDB; | ||
--latency-p50: #56CCF2; | ||
--subheader-grey: #828282; | ||
--status-gray: #BDBDBD; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some more references to this var in the codebase that should also be updated
Signed-off-by: Nathan Murthy <drip.dr0p.dr0p@gmail.com>
Just wanted to say it's nice to see you here, @natemurthy! |
hey @natemurthy thanks for the update! I think my comment in #147 (comment) got missed because it showed up in between your commits. if you address that, I think this branch is good to go! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✍️ 💥 shipit! thanks for the cleanup!
in the interest of making it into this release, I'll make a follow up branch to address the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me 🎉 (with @rmars follow-up branch to come addressing the last outstanding comment)
Thank you for contributing to conduit @natemurthy!!
Signed-off-by: Risha Mars <mars@buoyant.io>
Route labels are not queryable by tap, nor are they exposed to in tap events. This change uses the newly-added fields in linkerd/linkerd2-proxy-api#17 to make Tap route-aware.
@franziskagoltz For #135 is this what you were suggesting?