-
Notifications
You must be signed in to change notification settings - Fork 49
feat: Color Map pass the colorMapping object to giraffe
#3473
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
Conversation
colorMapping object to giraffe
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.
Good work! Code looks pretty clean and I like the comments you have. Have a couple of suggestions for more comments and maybe some more tests.
Edit: oh and like we said on Slack - let's hold off merging this until the start of next week
|
package.json
Outdated
| "cy:dev": "source ../monitor-ci/.env && CYPRESS_dexUrl=CLOUD CYPRESS_baseUrl=https://$INGRESS_HOST:$PORT_HTTPS cypress open --config testFiles='{cloud,shared}/**/*.*'", | ||
| "cy:dev-oss": "source ../monitor-ci/.env && CYPRESS_dexUrl=OSS CYPRESS_baseUrl=https://$INGRESS_HOST:$PORT_HTTPS cypress open --config testFiles='{oss,shared}/**/*.*'", | ||
| "generate": "export SHA=2da36d4beb7fc22dd8d037c3398c91c7ca2c346d && export REMOTE=https://raw.githubusercontent.com/influxdata/openapi/${SHA}/ && yarn generate-meta", | ||
| "generate": "export SHA=018d2381f78e52acbe1deaaff4c9512adf5e3703 && export REMOTE=https://raw.githubusercontent.com/influxdata/openapi/${SHA}/ && yarn generate-meta", |
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 SHA lives in a branch, maybe we should wait for this PR influxdata/openapi#248
| * 'result', | ||
| * ], | ||
| * | ||
| * the seriesID would be : "co-airSensors-TLM0102-mean-" |
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.
👍
| interpolation, | ||
| position: properties.position, | ||
| colors: colorHexes, | ||
| colorMapping, |
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.
an alternative to passing in colorMapping: null when the flag is off is doing something like:
remove this line in the definition of config, and then below where config is defined, doing this:
if (isFlagEnabled('graphColorMapping')) {
config.colorMapping = colorMapping
}to make sure this is truly 'dark' in the sense that it makes no changes to production code if the flag is off.
| @@ -0,0 +1,402 @@ | |||
| // tests will go here | |||
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.
✂️
Closes #3098
Bigger Picture:
The
translateis basically looking at theviewProperties.colorMappingobject from theidpeand then looking up thecolorusing the index in theproperties.colorsarray. We then embed thecolorproperty into themappings.[series_index]object in giraffe.Visualization of how the colorMapping object is translated into color for the giraffe. This translation happens in the UI. (
getColorMappingObjectsfunction)