villebro commented on code in PR #31751:
URL: https://github.com/apache/superset/pull/31751#discussion_r1962543516
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -61,6 +63,15 @@ import {
import { LabelLayout } from 'echarts/features';
import { EchartsHandler, EchartsProps, EchartsStylesProps } from '../types';
+// Define this interface here to avoid creating a dependency back to
superset-frontend,
+// and to appease the compiler.
+// ref: superset-frontend/src/explore/types
+interface ExplorePageState {
+ common: {
+ locale: string;
+ };
+}
Review Comment:
Oof.. I agree with the workaround here, but could we rephrase the comment to
a `// TODO:` to move the type to `@superset-ui/core`?
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -123,24 +134,52 @@ function Echart(
getEchartInstance: () => chartRef.current,
}));
+ const locale = useSelector(
+ (state: ExplorePageState) => state?.common?.locale ?? 'en',
Review Comment:
nit: Instead of having a magic string in the codebase, it would be nice to
move this to `src/constants.ts` as `DEFAULT_LOCALE = 'en';` or similar.
##########
superset-frontend/plugins/plugin-chart-echarts/package.json:
##########
@@ -24,9 +24,10 @@
"lib"
],
"dependencies": {
+ "@types/react-redux": "^7.1.10",
"d3-array": "^1.2.0",
- "lodash": "^4.17.21",
- "dayjs": "^1.11.13"
+ "dayjs": "^1.11.13",
+ "lodash": "^4.17.21"
Review Comment:
not your fault, but isn't `lodash` a peer dependency?
--
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]
