[GitHub] [superset] rusackas edited a comment on pull request #17792: feat: color consistency

[GitBox <gi...@apache.org>]

rusackas edited a comment on pull request #17792:
URL: https://github.com/apache/superset/pull/17792#issuecomment-1017110764


   Upon first glance at the test environment, I see that the dashboard I looked at was... consistent! Hooray! 👏 
   
   I also see that this solution works by effectively populating the JSON metadata of the dashboard with ALL the colors of ALL the series. I guess that works. That seems to cause a few potential issues:
   
   1) I'm not sure if storing the metadata here might cause any potential bloat issues for the data, if there's some crazy number of series on the dashboard
   2) If you go and delete the series colors from `shared_label_colors`, it doesn't seem like there's any way to get it back (adding more series, changing palettes, etc).
   3)  Normally people would add custom colors to `label_colors`, but it's now possible (and easy and discoverable) to edit the `shared_label_colors` object instead. This may lead to problems with these colors being overwritten when changing color palette.
   
   There are more cases I didn't test. For example, If you add a new series to two charts on a dashboard, hopefully those are also updated to match other existing charts. We should capture all the test cases for either manual or (better yet) automated tests to make sure it all works. Pinging @jinghua-qa to make her aware of the complexity here.
   
   In other words, this seems to work at first glance, but there are a few implications that make me nervous about the approach. I'm curious what @zhaoyongjie and/or @villebro might think of it.
   
   One solution to much of the above might be to forgo putting the `shared_label_colors` in the (user editable) json metadata, and instead using the dashboard's new key/value endpoint. Then it's in the database, and (hopefully?) could be edited repeatedly without the user even knowing it exists. I think it could still be accessed as needed by Explore, too, but I'm not sure.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org