search

heavyai / heavyai-charting

Dimensional charting built to work natively with crossfilter rendered using d3.js
https://heavyai.github.io/heavyai-charting/
Other
399 stars 73 forks source link

Restore previous legend behavior for most raster charts #594

Closed christopher-w-root closed 2 years ago

christopher-w-root commented 2 years ago

My windbarb pr https://github.com/heavyai/heavyai-charting/pull/585 largely broke legend behavior, particularly for what materialized into quantize scales. This was due to changes I made down in the stacked-legend code to assist with outputs from a more formal render-vega-lite-like interface that is WIP. The vega-lite-like interace is currently embedded only in the windbarbs, but there were a handful of places downstream that were impacted by having a new "state" structure. One of those was the stacked legend behavior. This restores the original behavior for charts that have not been upgraded to the new render-vega-lite iterface that I hope to incorporate. This is done by completely restoring the original LegendState code, and tagging the state input with a version. The old way will be tagged version 1 and will use the legacy LegendState. The new render-vega-lite way will be tagged v2, and will use the new logic behavior I've included. This is a stop gap to ensure all original logic is still in place to not cause further disruption.

I also matched how quantize scales are displayed in a legend with the legacy mode for added consistency.

Merge Checklist

:wrench: Issue(s) fixed:

:smoking: Smoke Test

:ship: Merge

christopher-w-root commented 2 years ago

@RavenHursT @johallar I've tagged you as reviewers here only because I've worked on PRs with you in the charting area recently. Not sure who else to ask, but I'd like to get this landed for the next RC and clear up some issues QA has seen with the most recent RC.

@RavenHursT - note the default legend display for color measures for wind barbs will be impacted by this. It is now more consistent with the other raster charts. Please let me know if you'd like to keep the existing look of the windbarb legend..

christopher-w-root commented 2 years ago

Seems like it may be a good idea to have an epic (or something) so we can try and get everything on v2 sooner than later for future dev sanity

probably

christopher-w-root commented 2 years ago

retest this please

christopher-w-root commented 2 years ago

Does anyone know how to trigger the mapd-charting check? I don't understand how to get it to fire.

johallar commented 2 years ago

For converting legend stuff: https://heavyai.atlassian.net/browse/FE-16028

johallar commented 2 years ago

Does anyone know how to trigger the mapd-charting check? I don't understand how to get it to fire.

This also foiled me... lemme know if you figure it out.

christopher-w-root commented 2 years ago

This also foiled me... lemme know if you figure it out.

It ultimately ran - some 2+ hours after initially opening the PR. Not sure what that's about -- almost feels like the jenkins job is on a cron rather than a push system.

If you look at the timestamps of the completed jobs, they seem to run about every hour if there's a PR in waiting: https://jenkins-os.mapd.com/job/mapd-charting/

yeah, if you look at the job configuration, it's set on an hourly cron

christopher-w-root commented 2 years ago

If you look at the notifications warning shield icon for jenkins-os, you see this in its long spew of warnings:

[Git plugin 4.10.3](https://plugins.jenkins.io/git)
[Multiple SCM plugins can check out from the controller file system](https://www.jenkins.io/security/advisory/2022-05-17/#SECURITY-2478)
[Lack of authentication mechanism in webhook](https://www.jenkins.io/security/advisory/2022-07-27/#SECURITY-284)
[Improper masking of credentials](https://www.jenkins.io/security/advisory/2022-08-23/#SECURITY-2796)

I bet that's the source of why various hooks do not trigger builds, like an opening of a PR or a 'retest this please' comment. Looks like jenkins-os needs some updating love to get all that working again.

christopher-w-root commented 2 years ago

@israelvicars et al.. What's the process for getting this into immerse now that it's landed?

RavenHursT commented 2 years ago

@vastcharade we just need to update immerse to point at the new HEAD commit of master here.

I can take care of that tomorrow if someone doesn't get to it before I do...