geido commented on pull request #17792: URL: https://github.com/apache/superset/pull/17792#issuecomment-1018554350
> 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. > I totally agree with this. I think we should hide the object from the user as discussed previously. @stephenLYZ this shouldn't be a big problem. We should be able to delete the object from the editor before its presented to the user. As for what concerns storing ALL labels, I think we agreed to only store the labels that are shared across different charts and not all of them. This might require a bit more logic in the code but it's important to avoid flooding the metadata. > 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. I think using the new key/value endpoint is a good idea. We could make sure the approach works correctly by using the metadata and then moving away from it in phase 2. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
