TimeSeriesWidget support for multiple time series. #767

zbigg · 2023-09-06T13:26:10Z

Description

Shortcut:

TimeSeriesWidget support for multiple time series. IN particular

support rendering of multiple series
support rendering for legend
support for new splitByCategory mode
support for filtering by category in splitByCategory mode
improved skeleton to better match sizing/placement of items with original (also in TableWidget)
new visuals in storybook

Type of change

Feature

Acceptance

Please describe how to validate the feature or fix

go to X
test Y
assert Z

If feature deals with theme / UI or internal elements used also in CARTO 3, please also add a note on how to do acceptance on that part.

Basic checklist

Good PR name
Shortcut link
Changelog entry
Just one issue per PR
GitHub labels
Proper status & reviewers
Tests
Documentation

shortcut-integration · 2023-09-06T13:26:13Z

This pull request has been linked to Shortcut Story #336297: C4R: Support new fields model for timeseries with multiple agg/splitByCategory/fidelity.

packages/react-ui/src/widgets/TimeSeriesWidgetUI/TimeSeriesWidgetUI.js

github-actions · 2023-09-06T13:37:16Z

Visit the preview URL for this PR (updated for commit 6df36a3):

https://cartodb-fb-storybook-react-dev--pr767-feature-sc-33629-gu0oee4n.web.app

_{(expires Mon, 09 Oct 2023 08:46:11 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 517cc4d31d7e09cf277774e034094b67c301cd4c}

shortcut-integration · 2023-09-06T13:37:22Z

This pull request has been linked to Shortcut Story #33629: Regenerar excels.

github-actions · 2023-09-06T16:24:01Z

Pull Request Test Coverage Report for Build 6225351711

120 of 248 (48.39%) changed or added relevant lines in 13 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-2.0%) to 69.52%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/react-ui/src/widgets/FormulaWidgetUI/FormulaSkeleton.js	0	1	0.0%
packages/react-ui/src/widgets/TableWidgetUI/Skeleton/TableSkeleton.js	3	4	75.0%
packages/react-ui/src/widgets/TimeSeriesWidgetUI/hooks/useTimeSeriesInteractivity.js	0	2	0.0%
packages/react-ui/src/widgets/TableWidgetUI/TableWidgetUI.js	3	6	50.0%
packages/react-ui/src/widgets/TimeSeriesWidgetUI/components/TimeSeriesSkeleton.js	8	14	57.14%
packages/react-ui/src/utils/palette.js	0	9	0.0%
packages/react-ui/src/widgets/TimeSeriesWidgetUI/TimeSeriesWidgetUI.js	21	33	63.64%
packages/react-ui/src/widgets/TimeSeriesWidgetUI/components/TimeSeriesChart.js	19	31	61.29%
packages/react-widgets/src/models/TimeSeriesModel.js	4	22	18.18%
packages/react-ui/src/widgets/TimeSeriesWidgetUI/components/TimeSeriesControls.js	45	76	59.21%

Files with Coverage Reduction	New Missed Lines	%
packages/react-ui/src/widgets/TimeSeriesWidgetUI/hooks/useTimeSeriesInteractivity.js	1	21.51%

Totals
Change from base Build 6182739362:	-2.0%
Covered Lines:	2312
Relevant Lines:	3067

💛 - Coveralls

packages/react-ui/src/widgets/TimeSeriesWidgetUI/TimeSeriesWidgetUI.js

Josmorsot

lgtm

mdeividas

in general, it looks good 👍

packages/react-ui/src/widgets/TimeSeriesWidgetUI/TimeSeriesWidgetUI.js

mdeividas · 2023-09-07T07:06:53Z

packages/react-ui/src/widgets/TimeSeriesWidgetUI/components/TimeSeriesControls.js

+  );
+}
+
+function ClockIcon() {


maybe it's worth considering to move those icons to separate files?

packages/react-ui/src/widgets/TimeSeriesWidgetUI/hooks/useTimeSeriesInteractivity.js

mdeividas · 2023-09-07T07:09:03Z

packages/react-widgets/src/hooks/useWidgetFilterValues.js

@@ -8,13 +8,14 @@ import { useMemo } from 'react';
 * @param  {object} props
 * @param  {string} props.dataSource - ID of the source to get the filters from.
 * @param  {string} props.id - ID of the widget that applied the filter.
- * @param  {string} props.column - name of column of this widget.
+ * @param  {string=} props.column - name of column of this widget.


do. you need this =? what does it mean?

it's means optional in JSDoc >> https://stackoverflow.com/questions/5873927/how-to-indicate-param-is-optional-using-inline-jsdoc

lerna.json

vmilan

LGTM, just consider the comment about color utils files

packages/react-ui/src/widgets/TimeSeriesWidgetUI/TimeSeriesWidgetUI.js

packages/react-ui/src/widgets/TimeSeriesWidgetUI/components/TimeSeriesLayout.js

packages/react-ui/src/widgets/utils/colorUtils.js

zbigg · 2023-09-14T08:14:01Z

packages/react-ui/src/widgets/TimeSeriesWidgetUI/components/TimeSeriesLayout.js

+  flexShrink: 0,
+  marginLeft: 0,
+  paddingLeft: theme.spacing(1),
+  [`@container (max-width: ${BREAKPOINTS.XS}px)`]: {


this fails in JSDOM with Error: Could not parse CSS stylesheet

zbigg force-pushed the feature/sc-336297/c4r-support-new-fields-model-for-timeseries branch from 4b8bccd to f899350 Compare September 6, 2023 13:33

zbigg marked this pull request as ready for review September 6, 2023 13:33

zbigg requested review from vmilan, Josmorsot, mdeividas, eamador and a team September 6, 2023 13:33

zbigg commented Sep 6, 2023

View reviewed changes

packages/react-ui/src/widgets/TimeSeriesWidgetUI/TimeSeriesWidgetUI.js Outdated Show resolved Hide resolved

zbigg force-pushed the feature/sc-336297/c4r-support-new-fields-model-for-timeseries branch 2 times, most recently from 3be3b61 to 07e6e1f Compare September 7, 2023 06:16

zbigg commented Sep 7, 2023

View reviewed changes

packages/react-ui/src/widgets/TimeSeriesWidgetUI/TimeSeriesWidgetUI.js Show resolved Hide resolved

Josmorsot approved these changes Sep 7, 2023

View reviewed changes

mdeividas approved these changes Sep 7, 2023

View reviewed changes

zbigg force-pushed the feature/sc-336297/c4r-support-new-fields-model-for-timeseries branch 3 times, most recently from dc6ab25 to 1e14024 Compare September 11, 2023 10:13

zbigg commented Sep 12, 2023

View reviewed changes

lerna.json Outdated Show resolved Hide resolved

zbigg force-pushed the feature/sc-336297/c4r-support-new-fields-model-for-timeseries branch 2 times, most recently from f6dd1b3 to 7660de0 Compare September 12, 2023 12:43

vmilan approved these changes Sep 14, 2023

View reviewed changes

zbigg commented Sep 14, 2023

View reviewed changes

zbigg force-pushed the feature/sc-336297/c4r-support-new-fields-model-for-timeseries branch 5 times, most recently from abc4908 to deae818 Compare September 18, 2023 17:04

zbigg added 26 commits October 2, 2023 10:36

skeleton to obey rowsperpage

2408c21

fix getMultipleSeries to work with new getTimeSeries model

efb543c

WidgetWithAlert with stable height for table/timeseries widget

3ab6473

increase padding in legend

45b8584

fix overflow in table skeleton

869a651

self CR

3bb91a8

typings for TimeSeries widget

2011829

remove null guard as backend is already fixed

91532d0

use stepMultipler for filter and for tooltip formatting

a088e6f

TimeSeriesWidget: fix clicking on line in single line mode

40d7e93

WrapperWidgetUI typings

c09a874

TimeSeriesWidget: fix clicking on line in single line mode even more

d33fb59

use common ChartLegend

3475cd1

fix TimeseriesWidgetSerie typing

05ffc05

TimeSeriesWidget: keep filters while re-loading

c7f2707

tooltip new date range formats

b5d2f2f

move date range format to separate file

4718b2f

fix support for several categoris with same name

b9e8b3a

fix crash when there are no categories

6aa2172

fix timeseries chart xAxis label styling

87fdd3b

derive splitNumber from widget width

e1f536e

xAxis fixes ...

ccddc22

TS: timeAxisSplitNumber as parameter

8c1c831

TableWidget: support onStateChange

0095685

fixup! TS: timeAxisSplitNumber as parameter

3ddb132

fix filtering when changing data

6df36a3

zbigg force-pushed the feature/sc-336297/c4r-support-new-fields-model-for-timeseries branch from 479cc6c to 6df36a3 Compare October 2, 2023 08:37

zbigg merged commit 567bcee into master Oct 2, 2023

zbigg deleted the feature/sc-336297/c4r-support-new-fields-model-for-timeseries branch October 2, 2023 08:47

zbigg mentioned this pull request Oct 2, 2023

Release 2.2.8 #781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TimeSeriesWidget support for multiple time series. #767

TimeSeriesWidget support for multiple time series. #767

zbigg commented Sep 6, 2023 •

edited

Loading

shortcut-integration bot commented Sep 6, 2023

github-actions bot commented Sep 6, 2023 •

edited

Loading

shortcut-integration bot commented Sep 6, 2023

github-actions bot commented Sep 6, 2023 •

edited

Loading

Josmorsot left a comment

mdeividas left a comment

mdeividas Sep 7, 2023

mdeividas Sep 7, 2023

zbigg Sep 7, 2023

vmilan left a comment

zbigg Sep 14, 2023

TimeSeriesWidget support for multiple time series. #767

TimeSeriesWidget support for multiple time series. #767

Conversation

zbigg commented Sep 6, 2023 • edited Loading

Description

Type of change

Acceptance

Basic checklist

shortcut-integration bot commented Sep 6, 2023

github-actions bot commented Sep 6, 2023 • edited Loading

shortcut-integration bot commented Sep 6, 2023

github-actions bot commented Sep 6, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6225351711

💛 - Coveralls

Josmorsot left a comment

Choose a reason for hiding this comment

mdeividas left a comment

Choose a reason for hiding this comment

mdeividas Sep 7, 2023

Choose a reason for hiding this comment

mdeividas Sep 7, 2023

Choose a reason for hiding this comment

zbigg Sep 7, 2023

Choose a reason for hiding this comment

vmilan left a comment

Choose a reason for hiding this comment

zbigg Sep 14, 2023

Choose a reason for hiding this comment

zbigg commented Sep 6, 2023 •

edited

Loading

github-actions bot commented Sep 6, 2023 •

edited

Loading

github-actions bot commented Sep 6, 2023 •

edited

Loading