Refactor the data flow inside of Scaler #313

evancharlton · 2019-05-30T11:38:28Z

Clean up the internal data flow inside of the Scaler component. This
refactoring had a few main goals:

Fully-own the responsibility of providing scaled data downstream
alongside the Series and Collection items.
Increase performance by using better checks to detect when the
incoming Series and Collections objects were externally changed.
Stop exposing domainsByItemId / subDomainsByItemId to the
downstream rendering components, in deference to Series and
Collection objects directly.
Get closer toward enabling per-Series timeDomains / timeSubDomains
as documented in Add possibility to have more than one time domains #301.

The end result of this is that the (internal) API has nontrivially
changed. Instead of Scaler exposing maps of domainsByItemId and
subDomainByItemId, it will simply pass through the series and
collections arrays, but they are now all guaranteed to be
fully-populated with domain and subDomain information. This means that
rendering components can simply render using the timeDomain,
timeSubDomain, xDomain, xSubDomain, yDomain, and ySubDomain domains
(which are guaranteed to be present) and don't need to concern
themselves with how they were populated (user/state, props, calculated
from data, or placeholders).

This does not yet fully enable separate timeDomain / timeSubdomain
throughout the library, but it removes almost all blockers. The final
piece to this puzzle is in DataProvider -- it needs to learn a one more
trick in order to support this (it currently only knows about one
timeSubDomain). That cleanup will be saved for a future PR.

cognite-cicd · 2019-05-30T19:32:31Z

[pr-server]
The storybook for this PR is hosted on https://griff-react-pr-313.surge.sh

Clean up the internal data flow inside of the Scaler component. This refactoring had a few main goals: 1. Fully-own the responsibility of providing scaled data downstream alongside the Series and Collection items. 2. Increase performance by using better checks to detect when the incoming Series and Collections objects were externally changed. 3. Stop exposing domainsByItemId / subDomainsByItemId to the downstream rendering components, in deference to Series and Collection objects directly. 4. Get closer toward enabling per-Series timeDomains / timeSubDomains as documented in #301. The end result of this is that the (internal) API has nontrivially changed. Instead of Scaler exposing maps of domainsByItemId and subDomainByItemId, it will simply pass through the series and collections arrays, but they are now all guaranteed to be fully-populated with domain and subDomain information. This means that rendering components can simply render using the timeDomain, timeSubDomain, xDomain, xSubDomain, yDomain, and ySubDomain domains (which are guaranteed to be present) and don't need to concern themselves with how they were populated (user/state, props, calculated from data, or placeholders). This does not yet fully enable separate timeDomain / timeSubdomain throughout the library, but it removes almost all blockers. The final piece to this puzzle is in DataProvider -- it needs to learn a one more trick in order to support this (it currently only knows about one timeSubDomain). That cleanup will be saved for a future PR.

Tweak .bulldozer.yaml to: - Use the PR description & title when merging - Respect [auto-merge] and [auto-update] labels - Response [do-not-merge] and [do-not-update]

I think I'm gonna introduce a final pipeline step

except for domains on collections

Or at least it seems to. Gotta update the storybook.

The step one is broken though

…er-flow

Also strip the empty collections

These aren't needed anymore.

Maybe this is what's going on with Jenkins

valgussev

Please don't forget to update readme at least about DataProvider

Stories:

http://localhost:9001/?path=/story/components-interactionlayer--area-x-zoom

Uncaught TypeError: Cannot read property 'priority' of null
    at domains.ts:61
    at Array.sort (<anonymous>)
    at highestPriorityDomain (domains.ts:60)
    at index.tsx:367
    at Array.map (<anonymous>)
    at Scaler._this.getSeriesWithDomains (index.tsx:347)
    at Scaler.render (index.tsx:540)
    at finishClassComponent (react-dom.development.js:14741)
    at updateClassComponent (react-dom.development.js:14696)
    at beginWork (react-dom.development.js:15644)

http://localhost:9001/?path=/story/linechart-live-loading--live-loading-and-ruler

If to hover over the chart, ruler jumped to the beginning of timeseries
If to zoom on context chart subdomain should follow the timsereries but currently it is sticky

http://localhost:9001/?path=/story/linechart-live-loading--limit-time-subdomain
sub domain is not limited

src/components/XAxis/index.tsx

valgussev · 2019-07-30T07:13:08Z

src/components/Griff/index.tsx

+  series: IncomingSeries[];
+  collections: IncomingCollection[];
+
+  children: JSX.Element | JSX.Element[];


Can't find onFetchData prop, has it been removed in older versions?

valgussev · 2019-07-30T07:23:41Z

src/components/Griff/index.tsx

+
+    return (
+      <Scaler
+        timeSubDomainChanged={() => null}


I probably missed that but I can't find on how it is used

valgussev · 2019-07-30T07:32:39Z

src/utils/domains.ts

+    return domain;
+  }
+  return newDomain(
+    extent[0] - diff * 0.025,


Just wondering what is the reasoning behind 0.025, 0.25, 1e-3?

This was just a buffer that "seemed right". I don't know the reasoning behind it.

valgussev · 2019-07-30T07:34:41Z

src/utils/placeholder.ts

+export const placeholder = (min: number, max: number): Domain =>
+  newDomain(min, max, DomainPriority.PLACEHOLDER);
+
+export const withoutPlaceholder = (


It seems everywhere withoutPlaceholder is used we return placeholder as a default value (i.e. withoutPlaceholder(c.xDomain) || PLACEHOLDER_DOMAIN), shouldn't it be renamed to something like withPlaceholder and take in placeholder as an argument?

valgussev · 2019-07-30T07:43:33Z

src/components/Scaler/index.tsx

@@ -1,123 +1,125 @@
 import * as React from 'react';
-import * as PropTypes from 'prop-types';
-import DataContext from '../../context/Data';


We should remove unused ../../context/Data

Good catch!

src/components/Joiner/index.tsx

src/components/DataProvider/index.tsx

valgussev · 2019-07-30T07:50:23Z

src/components/DataProvider/index.tsx

+    });
+  }
+
+  getLoaderReason = (series: ScaledSeries) => {


I don't see INTERVAL and UPDATE_POINTS_PER_SERIES have they been replaced or obsolete with a newer logic?

valgussev · 2019-07-30T07:52:37Z

stories/LineChart.stories.js

-    <DataProvider
-      defaultLoader={liveLoader}
+    <Griff
+      loader={liveLoader}


I believe entire functionality about isTimeSubDomainSticky is gone

evancharlton · 2019-08-02T06:28:37Z

Thanks for the review, @valerii-cognite ! I've gone ahead and updated the README and addressed some of the comments -- I'll continue to work on the others.

evancharlton requested a review from valgussev May 30, 2019 11:42

evancharlton force-pushed the scaler-flow branch 3 times, most recently from 2d9d905 to 8b01e76 Compare May 30, 2019 19:27

Evan Charlton added 25 commits May 30, 2019 21:41

Moved the comment

95049e9

Update .bulldozer.yaml

abfa136

Tweak .bulldozer.yaml to: - Use the PR description & title when merging - Respect [auto-merge] and [auto-update] labels - Response [do-not-merge] and [do-not-update]

Things are kinda working

d98b280

Almost done ripping out ScalerContext

6133c66

Hey neat things are even more kinda working

1e741ce

Time-boxed

b037c99

Whoa things are working

8644cbd

Porting LineChart.stories

1b942c3

Basic lines seem to be working

2fc7af0

Starting a big refactor

7d59ecf

Compiling cleanly, but some things are broken

68d7c61

Series working!

57bbe86

Collections mostly working, but the subdomains are off

6835063

I think I'm gonna introduce a final pipeline step

Wow the scaling and everything works

11b5f70

except for domains on collections

All of the domain + scaling stuff works!!

272cb53

Or at least it seems to. Gotta update the storybook.

Collection data domains working

7ad63c4

Added stories for the domain cases -- they're broken though

c78c801

Seems to work for series

9731b84

Collection domains working ... ?

9398e83

Everything works except the first collections domain case?

66adb81

Fix the single-value story

c1d3291

Fix the default accessors story

e8c15d3

Fix (& consolidate) min/max stories

a3e8a02

The step one is broken though

Fix step series

e4ef764

Evan Charlton added 4 commits July 14, 2019 18:01

Fix the compilation errors on Scatterplot stories

da3b08d

Get the Legacy API stories to not crash

91ed468

Make lint happy

acdf3bd

Merge branch 'master' of github.com:cognitedata/griff-react into scal…

64daf7c

…er-flow

evancharlton force-pushed the scaler-flow branch from 8b01e76 to 64daf7c Compare July 15, 2019 09:18

Evan Charlton added 16 commits July 17, 2019 10:00

Get yAxisDisplayMode working in AxisCollection

f7f2bac

Cleaned up the axis collection stories

cdc0804

Fix axis placements

b5169ef

Fix the padding on the ruler tooltip

20df327

Fix the collection yAccessor

ae015c0

Normalize the collection IDs before leaving Griff

b30dd46

Also strip the empty collections

Respect the zoomable ContextChart property

c248b45

Don't try to render GridLines without width & height

1384c11

Clean up some propTypes

dea5093

Remove propTypes from Scaler

a5003a8

These aren't needed anymore.

Fix scatterplots

04cddb6

Disable the x-axis placeholder

0b69866

Fix the scatterplot function loader

8c38424

Fix the function string IDs

c337375

More RAM when building storybook

2317f08

Run tsc before building storybook

9e4065e

Maybe this is what's going on with Jenkins

valgussev reviewed Jul 30, 2019

View reviewed changes

Evan Charlton added 5 commits August 2, 2019 08:09

Remove the legacy API

fa471d9

Remove unused Data

a361e1a

Reuse logic

e60e1c8

Update README

e76d2e1

Remove a console.log

f3077fd

evancharlton mentioned this pull request Nov 4, 2019

Upgrade storybook & dependencies #328

Merged

evancharlton closed this Jun 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the data flow inside of Scaler #313

Refactor the data flow inside of Scaler #313

evancharlton commented May 30, 2019 •

edited

Loading

cognite-cicd commented May 30, 2019

valgussev left a comment

valgussev Jul 30, 2019

valgussev Jul 30, 2019

valgussev Jul 30, 2019

evancharlton Aug 2, 2019

valgussev Jul 30, 2019

valgussev Jul 30, 2019

evancharlton Aug 2, 2019

valgussev Jul 30, 2019

valgussev Jul 30, 2019

evancharlton commented Aug 2, 2019

Refactor the data flow inside of Scaler #313

Refactor the data flow inside of Scaler #313

Conversation

evancharlton commented May 30, 2019 • edited Loading

cognite-cicd commented May 30, 2019

valgussev left a 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

evancharlton commented Aug 2, 2019

evancharlton commented May 30, 2019 •

edited

Loading