-
Notifications
You must be signed in to change notification settings - Fork 184
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
fix(treemap): assign leaf text color based on custom colors #1881
Conversation
- updated treemap chart to set the background color in data for the leaf text node so it can be referenced when determining the text color Fix carbon-design-system#1880
✅ Deploy Preview for carbon-charts-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-charts-angular ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-charts-core ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-charts-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thanks @SMassola, could you pls add a demo as well in the demo data so that this example is documented?
@@ -199,7 +200,7 @@ export class Treemap extends Component { | |||
return [ | |||
{ | |||
text: d.data.name, | |||
color: color.l < 0.5 ? 'white' : 'black' | |||
backgroundColor: color |
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.
seems like you removed the color
field here. is there a reason for that?
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.
The only reason I removed it was because it didn't seem to be utilized anywhere; if this is not the case I can revert this change.
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.
that sets the text fill based on backgroundColor
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 the color
property within the text node's data
wasn't being used; instead the text node's color is set by the textFillColor
function which utilizes either the new backgroundColor
property that I've added or the fill color of the parent rect.leaf
node.
The main issue was that when the textFillColor
called getComputedStyles
on the rect.leaf
node, the fill
property isn't set to the custom color early enough, so it always ended up determining the text color based on the default colors.
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.
yes @SMassola the text color values seem to be getting set properly in cases where you're providing custom colors, but if you don't, it seems like your PR has broken that case
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.
yes @SMassola the text color values seem to be getting set properly in cases where you're providing custom colors, but if you don't, it seems like your PR has broken that case
You're right, I've modified the code to send the backgroundColor
in the proper format (simply a string) to the textFillColor
function.
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.
Screen.Recording.2024-08-28.at.1.36.51.PM.mov
@@ -199,7 +200,7 @@ export class Treemap extends Component { | |||
return [ | |||
{ | |||
text: d.data.name, | |||
color: color.l < 0.5 ? 'white' : 'black' | |||
backgroundColor: color |
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.
that sets the text fill based on backgroundColor
I added an example to the docs which uses custom colors and appended a short demo video to the PR description. |
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.
Thanks @SMassola, just a final point, could you pls use a set of colors that offer larger contrast in your custom colors
demo?
some of these colors wouldn't pass a 3:1 test
I updated the chart colors to use teal 30-80 from the continuous monochromatic color palette (option 4). |
@theiliad this is what teal 30-80 looks like; let me know if this range of colors is appropriate: |
Fix #1880
Screen.Recording.2024-08-28.at.12.53.49.PM.mov