Antonio-RiveroMartnez commented on code in PR #33013:
URL: https://github.com/apache/superset/pull/33013#discussion_r2031507008
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx:
##########
@@ -36,6 +36,15 @@ import {
} from './types';
import { useOverflowDetection } from './useOverflowDetection';
+const MetricNameContainer = styled.div<{ metricNameFontSize?: number }>`
+ ${({ theme, metricNameFontSize }) => `
+ font-weight: ${theme.typography.weights.normal};
+ font-size: ${metricNameFontSize || 24}px;
Review Comment:
Would it be better to use the `theme.typography.sizes`?
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts:
##########
@@ -63,6 +67,28 @@ const config: ControlPanelConfig = {
config: { ...headerFontSize.config, default: 0.2 },
},
],
+ [
+ {
+ name: 'show_metric_name',
+ config: {
+ type: 'CheckboxControl',
+ label: t('Show Metric Name'),
+ renderTrigger: true,
+ default: true,
Review Comment:
Should be weird if existing charts start showing the metric all of the
sudden, wouldn't be better to default to false?
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts:
##########
@@ -50,6 +64,10 @@ const comparisonFontSizesMapping = getFontSizeMapping(
comparisonFontSizes,
);
+export const getMetricNameFontSize = (proportionValue: number) =>
Review Comment:
Not fault of this PR but we could DRY these utils a bit since all these
methods are pretty much the same but reading from different arrays:
metricNameFontSizes, headerFontSizes etc.
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberViz.tsx:
##########
@@ -293,6 +327,11 @@ class BigNumberVis extends
PureComponent<BigNumberVizProps> {
<div className={className}>
<div className="text-container" style={{ height: allTextHeight }}>
{this.renderFallbackWarning()}
+ {this.renderMetricName(
+ Math.ceil(
Review Comment:
Could we DRY this to render any info? because this is the same as how you're
rendering `kickerFontSize`
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts:
##########
@@ -94,6 +99,7 @@ export default function transformProps(chartProps:
ChartProps) {
const { data: dataA = [] } = queriesData[0];
const data = dataA;
const metricName = metric ? getMetricLabel(metric) : '';
+ const showMetricName = chartProps.rawFormData?.show_metric_name ?? true;
Review Comment:
Same comment about default from above
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts:
##########
@@ -16,10 +16,20 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { headerFontSize, subheaderFontSize } from '../sharedControls';
+import {
+ headerFontSize,
+ subheaderFontSize,
+ metricNameFontSize,
+} from '../sharedControls';
const headerFontSizes = [16, 20, 30, 48, 60];
const comparisonFontSizes = [16, 20, 26, 32, 40];
+const metricNameFontSizes = [12, 16, 20, 24, 28];
Review Comment:
Curious what's the header here? the big number? and probably we could use
the same sizes as the header or subheader?
--
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]
