JoshuaJADaniel commented on PR #36201: URL: https://github.com/apache/superset/pull/36201#issuecomment-3629550128
Appreciate the feedback @rusackas! > * It looks like this (pre-existing) control for labels/markdown isn't working right. Not sure if it does outside of this PR: [image]. But it _seems_ like it kind of covers both use cases of this PR? I'm not sure where that feature falls short. > I'm most curious about the first point, regarding the markdown controls, and if/how we should get those working, and whether or not that would solve the same purposes of rendering different properties, adding icons, etc. I do not think this PR impacted the tooltip functionality. In any case, the tooltip (presumably) only appears on hover. This is different from labels and icons, which remain visible without requiring a hover event. > * It looks like you can use Labels OR Icons, but not both, is that accurate? I'm wondering if a "Labels/Icons/None" Select would make more sense, and showing/hiding the relevant controls according to the option selected. You can use both labels and icons at the same time: <img width="1591" height="676" alt="image" src="https://github.com/user-attachments/assets/6a0bb27b-6ebb-45bf-808d-00f38d34e4f1"; /> If you are not using JavaScript though, the icon and label might overlap. > * If you choose settings for the image, and uncheck/re-check the "Image" box, the settings are wiped/lost. Would be nice to persist that. Agreed! Done in 44bd0d676478196622cd8886b92defa5c744f7dc. > * The icon size options seem sensible, but i wonder if we should allow a freeform entry there. It is actually free form, but it was not doing the string --> int conversion. Fixed in 8b83a50303e1edf1d4437430b9cf6a1ec648261f. > * If you have an icon selected/working, then check the "Text" option, it hides the icons and shows the text (awesome!) but does not show the icon again if you un-check "Text" > * Changing the settings for icons doesn't seem to refresh the viz, even though the controls seem to indicate that they should. You have to click Update... so something probably just isn't getting sent to TransformProps quite right or something like that. The second bullet here appears to be due to deck.gl's `IconLayer` failing to initialize when the icon URL is empty. When this happens, you have to hit "Update chart" to reinitialize the `IconLayer`. Fixed in f25f57ba3ad908f68744fcecf46c9211511f29c6. I was not able to reproduce the first bullet here, but I suspect it might be resolved with the same commit. -- 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]
