Skip to content

time & transforms #1019

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 1, 2022
Merged

time & transforms #1019

merged 4 commits into from
Aug 1, 2022

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jul 29, 2022

Save. (Not working, but getting closer.)

The tests are broken, they cover:

  • gapminder-box.js : transforms (almost working, but some demultiplication of marks appears)
  • gapminder-box-facet.js : faceted transforms (broken rects)
  • gampinder-dodge.js : initializers (broken: it should not pile up all the dots from different time frames)

@Fil Fil requested a review from mbostock July 29, 2022 13:46
@Fil Fil mentioned this pull request Jul 29, 2022
12 tasks
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for exploring this. I think even though you got the facets work in the other PR #1018 we should probably jump ahead to something like this approach. I think the farther we go down the filter-on-render approach, the more we have to unwind when we switch to the facet approach that will be needed to support transforms.

Rather than doing this in mark.initialize, I think we’ll have to do this up a higher level in Plot.plot, before mark.initialize is called (so that at that point, the facets are already split by time). This way the mark doesn’t need to understand how the facets are derived, which better isolates the logic and makes it backwards compatible and maybe we’ll think of other faceting logic in the future, too.

@Fil Fil merged commit 6005af8 into fil/time Aug 1, 2022
@Fil Fil deleted the fil/time-facets branch August 1, 2022 17:46
@Fil
Copy link
Contributor Author

Fil commented Aug 1, 2022

continuing at #1018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants