refactor: implement repeat as a normalization to reduce number of models #6002

domoritz · 2020-03-02T01:49:57Z

Fixes #4947

This also fixes the layer type, which incorrectly extended generic layer spec.

Fixes #4947

domoritz · 2020-03-05T18:21:51Z

Rebased to master. Looking pretty good now.

examples/compiled/geo_repeat.vg.json

kanitw · 2020-03-06T06:18:09Z

Let's rebase after #6026 is merged.

domoritz · 2020-03-06T20:31:15Z

TODO:

~~Make sure we still use the repeat config~~
Clean up extractCompositionLayout

kanitw · 2020-03-09T00:00:08Z

I think you just caused even more renames lol

domoritz · 2020-03-09T01:28:27Z

I think it's only these ones, right?

kanitw · 2020-03-09T02:13:18Z

I think it's only these ones, right?

IDK, because I haven't looked at them -- too many.

domoritz · 2020-03-10T02:48:19Z

I looked through the generated SVGs and this looks good to me. Anything left @kanitw?

kanitw · 2020-03-10T04:07:41Z

examples/compiled/interactive_layered_crossfilter_discrete.vg.json

@@ -183,6 +182,7 @@
  ],
  "signals": [
    {"name": "childWidth", "value": 200},
+    {"name": "childHeight", "value": 200},


Why does this become childHeight? Similar to many other specs. This is repeat-column, which means the height can be reasonably shared.

Do you see an issue in the normalized (concat) spec? The height here is just the default, no?

kanitw · 2020-03-10T04:10:02Z

I looked through the generated SVGs and this looks good to me. Anything left @kanitw?

It'd be great to look at generated Vega too.

Just added one comment after looking at a few Vega specs.

test/compile/repeat.test.ts

src/spec/map.ts

src/log/message.ts

src/compile/unit.ts

kanitw · 2020-03-10T17:41:03Z

src/compile/selection/transforms/scales.ts

@@ -34,12 +34,6 @@ const scaleBindings: TransformCompiler = {
      const extent = {selection: selCmpt.name, field: proj.field};
      scale.set('selectionExtent', extent, true);
      bound.push(proj);
-
-      // Bind both x/y for diag plot of repeated views.
-      if (model.repeater && model.repeater.row === model.repeater.column) {


this seems problematic?

This is #6000, right?

Perhaps, but what's the effect of removing these lines?

Let's try: #6124

scripts/rename-schema.sh

src/normalize/core.ts

examples/compiled/repeat_splom_cars.vg.json

kanitw

I'm glad to try to isolate massive diff so we can detect this small thing

examples/compiled/interactive_panzoom_splom.vg.json

kanitw · 2020-03-22T18:35:16Z

src/normalize/core.ts

    return {
-      ...spec,
-      spec: this.map(spec.spec, params)
+      data: data ?? childSpec.data, // data from child spec should have precedence


The code here doesn't really give precedence to child spec?

kanitw · 2020-03-22T18:37:53Z

src/normalize/core.ts

+            'child' +
+            (repeatValue ? `__repeat_repeat_${varName(repeatValue)}` : '') +
+            (rowValue ? `__repeat_row_${varName(rowValue)}` : '') +
+            (columnValue ? `__repeat_column_${varName(columnValue)}` : '');


The naming logic here seems wrong for nested repeat?

Existing repeatValue should be treated as prefix to the name, not inserted in the middle.

domoritz requested review from arvind and kanitw March 2, 2020 01:49

domoritz force-pushed the dom/repeat-macro branch from 9329da1 to 252a71b Compare March 2, 2020 02:26

domoritz marked this pull request as ready for review March 2, 2020 02:26

domoritz force-pushed the dom/repeat-macro branch from 252a71b to 449e1af Compare March 2, 2020 20:54

domoritz added 2 commits March 5, 2020 10:17

refactor: implement repeat as a normalization to reduce number of models

95fe39d

Fixes #4947

fix: correct layer spec type to be circular

5899f3e

domoritz force-pushed the dom/repeat-macro branch from 10cb260 to 5899f3e Compare March 5, 2020 18:21

domoritz mentioned this pull request Mar 5, 2020

Remove GenericLayerSpec #6022

Open

chore: simplify schema

17c1e81

domoritz requested a review from a team March 5, 2020 18:50

fix: repeated views should be aligned by default

199e00e

domoritz force-pushed the dom/repeat-macro branch from d41eb83 to 17c1e81 Compare March 5, 2020 18:53

kanitw requested changes Mar 5, 2020

View reviewed changes

examples/compiled/geo_repeat.vg.json Outdated Show resolved Hide resolved

examples/compiled/geo_repeat.vg.json Outdated Show resolved Hide resolved

domoritz added 2 commits March 5, 2020 11:33

refactor: do not use simpler names for now

97fb4ef

feat: recognize vconcat and hconcat for layoutsize

9e5370d

domoritz force-pushed the dom/repeat-macro branch from ac36746 to 9e5370d Compare March 5, 2020 23:43

domoritz commented Mar 6, 2020

View reviewed changes

examples/compiled/geo_repeat.vg.json Outdated Show resolved Hide resolved

kanitw reviewed Mar 6, 2020

View reviewed changes

examples/compiled/geo_repeat.vg.json Outdated Show resolved Hide resolved

refactor: use previous signal names

a0233ad

domoritz force-pushed the dom/repeat-macro branch from a84080e to a0233ad Compare March 6, 2020 01:36

domoritz commented Mar 6, 2020

View reviewed changes

examples/compiled/geo_repeat.vg.json Outdated Show resolved Hide resolved

domoritz requested a review from kanitw March 6, 2020 02:07

This was referenced Mar 6, 2020

refactor: implement repeat as a normalization to reduce number of models #6025

Closed

fix: make layoutsize always merge width and height #6026

Closed

kanitw added the Blocked 🕐 For issues that are blocked by other issues label Mar 6, 2020

kanitw removed their request for review March 6, 2020 22:56

domoritz added 3 commits March 9, 2020 16:58

Merge branch 'master' into dom/repeat-macro

13c150f

chore: update runtime examples

2132e21

chore: rebuild toc

655f1fd

domoritz force-pushed the dom/repeat-macro branch from a31f229 to 655f1fd Compare March 10, 2020 00:35

kanitw reviewed Mar 10, 2020

View reviewed changes

examples/compiled/repeat_splom_cars.vg.json Outdated Show resolved Hide resolved

domoritz added 2 commits March 10, 2020 11:28

chore: address comments

496a556

Merge branch 'master' into dom/repeat-macro

bcf9b8b

domoritz force-pushed the dom/repeat-macro branch from 882323d to bcf9b8b Compare March 10, 2020 18:29

domoritz added 2 commits March 10, 2020 11:36

refactor: move repeat to normalize

e2b69ab

refactor: remove normalizedEncoding variable

6d4721d

domoritz force-pushed the dom/repeat-macro branch from 844711f to 6d4721d Compare March 10, 2020 18:38

kanitw mentioned this pull request Mar 15, 2020

fix: make concat/repeat avoid using width/height #6095

Merged

kanitw reviewed Mar 15, 2020

View reviewed changes

examples/compiled/interactive_panzoom_splom.vg.json Show resolved Hide resolved

kanitw mentioned this pull request Mar 16, 2020

Refactor Repeat as a Macro? #4947

Closed

Merge branch 'master' into dom/repeat-macro

c47962e

domoritz force-pushed the dom/repeat-macro branch from 3b70a14 to c47962e Compare March 17, 2020 21:16

chore: update examples [CI]

78fc800

domoritz merged commit 5c0b299 into master Mar 17, 2020

domoritz deleted the dom/repeat-macro branch March 17, 2020 22:26

kanitw reviewed Mar 22, 2020

View reviewed changes

kanitw mentioned this pull request Mar 22, 2020

fix: correct repeat child name logic and data precedence #6141

Merged

kanitw mentioned this pull request Aug 31, 2020

Schema shouldn't include normalized spec / FieldName #6823

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: implement repeat as a normalization to reduce number of models #6002

refactor: implement repeat as a normalization to reduce number of models #6002

domoritz commented Mar 2, 2020 •

edited

Loading

domoritz commented Mar 5, 2020

kanitw commented Mar 6, 2020 •

edited

Loading

domoritz commented Mar 6, 2020 •

edited by kanitw

Loading

kanitw commented Mar 9, 2020

domoritz commented Mar 9, 2020

kanitw commented Mar 9, 2020

domoritz commented Mar 10, 2020

kanitw Mar 10, 2020 •

edited

Loading

domoritz Mar 10, 2020

kanitw commented Mar 10, 2020

kanitw Mar 10, 2020

domoritz Mar 10, 2020

kanitw Mar 10, 2020 •

edited

Loading

domoritz Mar 17, 2020

kanitw left a comment

kanitw Mar 22, 2020

kanitw Mar 22, 2020

refactor: implement repeat as a normalization to reduce number of models #6002

refactor: implement repeat as a normalization to reduce number of models #6002

Conversation

domoritz commented Mar 2, 2020 • edited Loading

domoritz commented Mar 5, 2020

kanitw commented Mar 6, 2020 • edited Loading

domoritz commented Mar 6, 2020 • edited by kanitw Loading

kanitw commented Mar 9, 2020

domoritz commented Mar 9, 2020

kanitw commented Mar 9, 2020

domoritz commented Mar 10, 2020

kanitw Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

domoritz Mar 10, 2020

Choose a reason for hiding this comment

kanitw commented Mar 10, 2020

kanitw Mar 10, 2020

Choose a reason for hiding this comment

domoritz Mar 10, 2020

Choose a reason for hiding this comment

kanitw Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

domoritz Mar 17, 2020

Choose a reason for hiding this comment

kanitw left a comment

Choose a reason for hiding this comment

kanitw Mar 22, 2020

Choose a reason for hiding this comment

kanitw Mar 22, 2020

Choose a reason for hiding this comment

domoritz commented Mar 2, 2020 •

edited

Loading

kanitw commented Mar 6, 2020 •

edited

Loading

domoritz commented Mar 6, 2020 •

edited by kanitw

Loading

kanitw Mar 10, 2020 •

edited

Loading

kanitw Mar 10, 2020 •

edited

Loading