-
Notifications
You must be signed in to change notification settings - Fork 162
Add Support for Heatmap Plot Type in CVD Palette Feature #950 #951
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 Support for Heatmap Plot Type in CVD Palette Feature #950 #951
Conversation
…r e InternetHealthReport#950 I make minimum change as much as possible in colorPalettes
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.
This PR does not adhere to the code style established for adding a new plot type. Additionally, it modifies the CVD color palettes, whereas it was previously discussed that any changes to the CVD palettes should be made using a new variable rather than altering the existing one. This PR requires significant revisions.
Note: This PR appears to be generated by an LLM.
@@ -73,7 +73,7 @@ const created = ref(false) | |||
const myId = ref(`ihrReactiveChart${uid()}`) | |||
const layoutLocal = ref(props.layout) | |||
const dropdownCVD = ref(false) | |||
const supportedCVDPlots = ['treemap', 'pie', 'bar', 'box', 'scatter', 'scatterpolar'] | |||
const supportedCVDPlots = ['treemap', 'pie', 'bar', 'box', 'scatter', 'scatterpolar', 'heatmap'] |
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.
Add heatmap
third in the array
@@ -92,7 +92,7 @@ layoutLocal.value['images'] = [ | |||
|
|||
const colorPalettes = { | |||
protanopia: [ | |||
'#ffe41c', | |||
'#ffe41c', //Light Yellow |
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.
remove comment
@@ -104,7 +104,7 @@ const colorPalettes = { | |||
'#7e711b', | |||
'#000000', | |||
'#0060c7', | |||
'#a18e21', | |||
'#a18e21', // dark yellow |
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.
remove comment
@@ -114,10 +114,10 @@ const colorPalettes = { | |||
'#686566' | |||
], | |||
deuteranopia: [ | |||
'#ffd592', | |||
'#679bf2', //Light blue |
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.
remove 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.
Revert to the old color code
'#b0bcf9', | ||
'#c09300', | ||
'#679bf2', | ||
'#ffd592', |
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.
Revert to the old color code
@@ -136,7 +136,8 @@ const colorPalettes = { | |||
'#8894ca' | |||
], | |||
tritanopia: [ | |||
'#fd6e74', | |||
'#ed656c ', //light pink | |||
//'#ffc0cd ', |
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.
remove line
@@ -148,10 +149,10 @@ const colorPalettes = { | |||
'#ffc0cd', | |||
'#67becd', | |||
'#ff4346', | |||
'#36aebb', | |||
'#d46269', //dark pink |
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.
revert to old color code
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.
remove comment
'#ed656c', | ||
'#4d717a', | ||
'#d46269', | ||
'#36aebb', |
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.
revert to old color code
} | ||
} else if (trace.type === supportedCVDPlots[1]) { | ||
const colorsArray = colorPalettes[paletteKey]; | ||
if (trace.type === 'heatmap') { |
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.
Change to trace.type === supportedCVDPlots[3]
and add this part of before the else part.
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.
Why did you delete the const color = colorPalettes[paletteKey][index % colorPalettes[paletteKey].length]
?
return { | ||
...trace, | ||
marker: { | ||
...trace.marker, | ||
colors: colorsArray.slice(0, trace.labels?.length || colorsArray.length) | ||
} | ||
} | ||
} else { | ||
return { | ||
...trace, | ||
marker: { ...trace.marker, color }, | ||
line: { ...trace.line, color } | ||
colorscale: [ | ||
[0, '#dddbd9'], // start with white | ||
[0.5, colorsArray[0]], // Light color | ||
[1, colorsArray[Math.floor(colorsArray.length * 0.6)]] // Strong 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.
This code block is not align with the code style of the file
Thanks for reviewing my PR! After going through your feedback, I re-evaluated my approach:
Your comment on this line caught my attention:
Then applied it here:
and the result was same Does this approach make sense to you? If it looks good, I'll proceed with the next steps. Let me know if you think there's a better way to handle it! |
I have some questions:
|
|
I think you haven't understand my questions;
|
|
Ok, please remove the shade function, as it is unnecessary. You can simply declare the corresponding colors in constants, including the light version. For example: const colorPalettesScaling = {
protanopia: {
light: '#ffe41c',
dark: 'dark hex'
},
// etc.
}; Please adjust your code according to my comments and push the changes again. Thank you. |
- Implemented static color mapping using `colorPalettesScaling` - Adjusted the heatmap logic to use predefined light/dark colors - Ensured the new approach aligns with the requested changes in PR InternetHealthReport#951
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
Description
This PR enhances the heatmap color scaling to provide a more visually accurate and accessible experience for users with color vision deficiencies (CVD). The key improvements include:
Changes:-
Why is this change required? What problem does it solve?
Related issue
Closes #950 (Add Support for Heatmap Plot Type in CVD Palette Feature)
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: