feat: add new plugin to generate period over period KPI charts #847

fernando-bytecode · 2024-01-23T18:38:05Z

SUMMARY

Creating a new chart that allows arbitrary time range comparisons for Key Performance Indicators (KPI). This follows similar multi-value visualizations found in other BI tools.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Create new chart
Select Period Over Period KPI as the Chart type under Other
Use the cleaned_sales dataset for testing using sales as a measure.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

github-actions

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

craig-rueda · 2024-01-24T00:47:25Z

Please make this PR against https://github.com/apache/superset as this fork is rebased from there :)

eschutho · 2024-01-24T00:47:55Z

This is just for testing and cursory review

geido · 2024-01-24T10:41:38Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

+// imported from @superset-ui/core. For variables available, please visit
+// https://github.com/apache-superset/superset-ui/blob/master/packages/superset-ui-core/src/style/index.ts
+
+const Styles = styled.div<PopKPIStylesProps>`


For consistency, can we use css here while defining the styles as a variable as detailed in the wiki

geido · 2024-01-24T10:42:28Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

+  display: flex;
+  flex-direction: column;
+  justify-content: center;
+  padding: ${({ theme }) => theme.tdUnit * 4}px;


First time I see this tdUnit, what about gridUnit instead?

geido · 2024-01-24T10:43:44Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

+  width: ${({ width }) => width}px;
+`;
+
+const BigValueContainer = styled.div`


Same as above, let's use css for styled component that do not need to be re-used, either inline or by assigning them to a var

geido · 2024-01-24T10:45:37Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

+// The following Styles component is a <div> element, which has been styled using Emotion
+// For docs, visit https://emotion.sh/docs/styled
+
+// Theming variables are provided for your use via a ThemeProvider
+// imported from @superset-ui/core. For variables available, please visit
+// https://github.com/apache-superset/superset-ui/blob/master/packages/superset-ui-core/src/style/index.ts
+


Suggested change

// The following Styles component is a <div> element, which has been styled using Emotion

// For docs, visit https://emotion.sh/docs/styled

// Theming variables are provided for your use via a ThemeProvider

// imported from @superset-ui/core. For variables available, please visit

// https://github.com/apache-superset/superset-ui/blob/master/packages/superset-ui-core/src/style/index.ts

geido · 2024-01-24T10:47:34Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

+
+const Styles = styled.div<PopKPIStylesProps>`
+
+  font-family: ${({ theme }) => theme.typography.families.sansSerif};


Please refer to No need to use theme the hard way

geido · 2024-01-24T10:54:48Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/controlPanel.ts

+              // it's possible to add validators to controls if
+              // certain selections/types need to be enforced


Suggested change

// it's possible to add validators to controls if

// certain selections/types need to be enforced

geido · 2024-01-24T10:57:10Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/controlPanel.ts

+              ...sharedControls.adhoc_filters,
+              label: t('FILTERS (Custom)'),
+              description:
+                'This only applies when selecting the Range for Comparison Type- Custom',


Please localize this

geido · 2024-01-24T10:59:01Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/index.ts

+   */
+  constructor() {
+    const metadata = new ChartMetadata({
+      description: 'KPI viz for comparing multiple period',


Let's localize this

geido · 2024-01-24T10:59:33Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/transformProps.ts

+    width,
+    height,
+    data,
+    // and now your control data, manipulated as needed, and passed through as props!


Suggested change

// and now your control data, manipulated as needed, and passed through as props!

geido · 2024-01-24T11:03:00Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/buildQuery.ts

+
+  let modTimeRange: string | null = timeRange;
+
+  if (timeRange === 'NO_TIME_RANGE' || timeRange === '_(NO_TIME_RANGE)'){


Are you sure these are supposed to be strings and not a constant? I am looking at superset/utils/date_parser.py and it looks like these are being referred to a constant NO_TIME_RANGE = "No filter"

geido · 2024-01-24T11:11:17Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/buildQuery.ts

+
+type MomentTuple = [moment.Moment | null, moment.Moment | null];
+
+function getSinceUntil(


It seems like this function does not belong here but to the utils. Maybe time-format in superset-ui-core?

geido · 2024-01-24T11:19:01Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/buildQuery.ts

+  );
+
+  const [testSince, testUntil] = getSinceUntil(
+    timeFilter.comparator.toLowerCase(),


Should we add a null check here?

geido · 2024-01-24T11:23:49Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/buildQuery.ts

+    startDatePrev = startDate.subtract(1, 'month');
+    endDatePrev = endDate.subtract(1, 'month');
+  } else if (calcType === 'r') {
+    startDatePrev = startDate.clone().subtract(daysBetween.valueOf(), 'day');


diff should already return a number so valueOf might be redundant here

Do we need clone here?

kgabryje · 2024-01-24T12:05:45Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

+
+  const rootElem = createRef<HTMLDivElement>();
+
+  useEffect(() => {


What is this for? I don't think we use it anywhere?

kgabryje · 2024-01-24T12:06:57Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/index.ts

+ * either add additional plugin folders (similar in structure to ./plugin)
+ * OR export multiple instances of `ChartPlugin` extensions in ./plugin/index.ts
+ * which in turn load exports from CustomViz.tsx
+ */


I don't think we need all those comments (in other files too)

kgabryje · 2024-01-24T12:12:13Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/buildQuery.ts

+    return [null, null];
+  }
+
+  if (timeRange?.startsWith('last') && !timeRange.includes(separator)) {


This logic is a copy of superset/utils/date_parser.py::get_since_until right? If that's the case, could you add a comment here that refers to date_parser.py, and the other way around - a comment in date_parser.py that refers to this function. The goal is that when someone tries to make a change to 1 of those functions, they should know that they need to sync that change with the corresponding function

kgabryje · 2024-01-24T12:14:29Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/buildQuery.ts

+  ]);
+
+
+  const timeFilter: any = formData.adhoc_filters?.find(


Wherever possible, please try not to use any

kgabryje · 2024-01-24T12:16:21Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/buildQuery.ts

+
+  let formDataB: QueryFormData;
+
+  if (timeComparison!='c'){


You can run your code through prettier (npm run format) to format it to match the style of other files

kgabryje · 2024-01-24T12:20:28Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/controlPanel.ts

+        [
+          {
+            name: 'metrics',
+            config: {


It's not necessary to override the config, metrics control already has non empty validator set. You can just put ['metrics'] here

kgabryje · 2024-01-24T12:21:17Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/controlPanel.ts

+              label: t('Range for Comparison'),
+              default: 'y',
+              choices: [
+                ['y', 'Year'],


We should also localize the names here

kgabryje · 2024-01-24T12:26:26Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/transformProps.ts

+  const dataA = queriesData[0].data as TimeseriesDataRecord[];
+  const dataB = queriesData[1].data as TimeseriesDataRecord[];
+  const data = dataA;
+  const metricName = getMetricLabel(metrics[0]);


Do we only take the first metric here and ignore the rest? If so, we should use metric control instead of metrics in the control panel

kgabryje · 2024-01-24T12:29:37Z

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/transformProps.ts

+  const percentDifference: string = formatPercentChange(percentDifferenceNum);
+
+  return {
+    width,


We should only return those values that are used in the main plugin file. I see that don't use for example data, metrics or metricName in PopKPI so we should remove those from here

eschutho · 2024-01-30T03:44:32Z

@fernando-bytecode I'm going to merge this PR into my branch so that we can add a feature flag on it but go ahead and open a new PR to it when you're ready with the Ci fixes,etc.

Period Over Period KPI plugin

b782d9a

github-actions bot reviewed Jan 23, 2024

View reviewed changes

eschutho force-pushed the bytecode/time-shift branch from 6a1ce6a to 3ee98e9 Compare January 24, 2024 00:42

fernando-bytecode requested review from geido, eschutho, betodealmeida, mistercrunch, craig-rueda, kgabryje and dpgaspar as code owners January 24, 2024 00:42

eschutho force-pushed the bytecode/time-shift branch from 3ee98e9 to 6a1ce6a Compare January 24, 2024 00:44

craig-rueda closed this Jan 24, 2024

eschutho reopened this Jan 24, 2024

geido reviewed Jan 24, 2024

View reviewed changes

kgabryje reviewed Jan 24, 2024

View reviewed changes

eschutho merged commit 68fe17f into preset-io:bytecode/time-shift Jan 30, 2024
43 of 55 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new plugin to generate period over period KPI charts #847

feat: add new plugin to generate period over period KPI charts #847

fernando-bytecode commented Jan 23, 2024

github-actions bot left a comment

craig-rueda commented Jan 24, 2024

eschutho commented Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

geido Jan 24, 2024

kgabryje Jan 24, 2024

kgabryje Jan 24, 2024

kgabryje Jan 24, 2024 •

edited

Loading

kgabryje Jan 24, 2024

kgabryje Jan 24, 2024

kgabryje Jan 24, 2024 •

edited

Loading

kgabryje Jan 24, 2024

kgabryje Jan 24, 2024

kgabryje Jan 24, 2024

kgabryje Jan 24, 2024

eschutho commented Jan 30, 2024


		const Styles = styled.div<PopKPIStylesProps>`

		font-family: ${({ theme }) => theme.typography.families.sansSerif};

		// it's possible to add validators to controls if
		// certain selections/types need to be enforced


		let modTimeRange: string \| null = timeRange;

		if (timeRange === 'NO_TIME_RANGE' \|\| timeRange === '_(NO_TIME_RANGE)'){


		type MomentTuple = [moment.Moment \| null, moment.Moment \| null];

		function getSinceUntil(


		const rootElem = createRef<HTMLDivElement>();

		useEffect(() => {

feat: add new plugin to generate period over period KPI charts #847

feat: add new plugin to generate period over period KPI charts #847

Conversation

fernando-bytecode commented Jan 23, 2024

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

github-actions bot left a comment

Choose a reason for hiding this comment

craig-rueda commented Jan 24, 2024

eschutho commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho commented Jan 30, 2024

kgabryje Jan 24, 2024 •

edited

Loading

kgabryje Jan 24, 2024 •

edited

Loading