ramiroaquinoromero commented on PR #37256:
URL: https://github.com/apache/superset/pull/37256#issuecomment-3776175640
> @ramiroaquinoromero The before screenshot already shows the multi chart
icon disappearing when not selected. Am i not understanding the problem
correctly? And adding a function just for a single chart doesn't seem like the
right solution
Thanks for the feedback @msyavuz, you're right the before screenshot shows
multi chart disappearing when not selected. That behavior was already
implemented correctly so I focused on implement the other things that werent
implemented so what this PR actually fixes is that multi chart should show its
custom figma icon. There was a bug that when multi chart was selected, it
showed generic fallback icons instead of the custom figma design:
* MonitorOutlined (monitor icon) when selected from modal
* CheckSquareOutlined (checkbox icon) when active
This happened because Multi Chart is not in FEATURED_CHARTS, so it fell
through to the generic fallback icons, and to fix this I added conditional
checks for deck_multi to use Icons.Multiple (the custom Figma icon) instead of
the generic fallbacks.
Regarding the helper function getMultiChartIcon, you're right that it's a
small helper for a single chart, I added it because the icon is used in two
places:
* [line
59](https://github.com/apache/superset/pull/37256/files#diff-ba6b13f05cb808abc92fac5ed9120b16358dcf58978d23efee89cd6da17ca32aR59)
: When Multi Chart is selected from the "View all charts" modal (preview
state, before clicking "Update chart")
* [line
78](https://github.com/apache/superset/pull/37256/files#diff-ba6b13f05cb808abc92fac5ed9120b16358dcf58978d23efee89cd6da17ca32aR78)
: When Multi Chart is the active/rendered chart (after clicking "Update chart")
Both need the same icon with the same viewBox="5 4 15 20" adjustment for
proper alignment.
The viewBox adjustment is specific to Multi Chart's icon alignment, so
centralizing it avoids duplication. That said, if you'd prefer, I can remove
the helper function and just add <Icons.Multiple iconSize="l" viewBox="5 4 15
20" /> directly in both places instead.
--
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]
