kgabryje commented on code in PR #27052:
URL: https://github.com/apache/superset/pull/27052#discussion_r1483166664
##########
superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx:
##########
@@ -30,16 +34,51 @@ const ComparisonValue =
styled.div<PopKPIComparisonValueStyleProps>`
`}
`;
+const SymbolWrapper = styled.div<PopKPIComparisonSymbolStyleProps>`
+ ${({ theme, percentDifferenceNumber, comparisonColorEnabled }) => {
+ const defaultBackgroundColor = theme.colors.grayscale.light4;
Review Comment:
Interesting approach with putting js inside of styled component like that!
But I wonder if deducing colours from more "high-level" data belongs to a
styled component. Maybe we should move that logic to `PopKPI` and just pass
`backgroundColor` and `textColor` to `SymbolWrapper`?
But if you think this solution is better then I won't argue, I don't have a
strong opinion here
--
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-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
