Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨ Data pages: enable rendering of data pages on all eligible data pages, remove old datapage #2848

Merged
merged 3 commits into from
Nov 27, 2023
Merged

✨ Data pages: enable rendering of data pages on all eligible data pages, remove old datapage #2848

merged 3 commits into from
Nov 27, 2023

Conversation

danyx23
Copy link
Contributor

@danyx23 danyx23 commented Oct 24, 2023 •
edited
Loading

This PR removes the old prototype data page code and switches rendering of eligible charts to show data pages.

☣️ should only be merged once the team is happy with this as it will start rendering data pages on public /grapher/ urls

@danyx23 danyx23 force-pushed the data-page-bake-on-all-eligible-charts-from-etl branch 2 times, most recently from fd8f210 to 2e669d1 Compare October 25, 2023 09:16
@sophiamersmann sophiamersmann mentioned this pull request Oct 27, 2023
Merged
@danyx23 danyx23 force-pushed the data-page-bake-on-all-eligible-charts-from-etl branch from 50ac8bf to 2b18987 Compare October 31, 2023 08:15
@danyx23 Graphite App
Copy link
Contributor Author

danyx23 commented Oct 31, 2023 •
edited
Loading

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@danyx23 danyx23 mentioned this pull request Oct 31, 2023
Merged
1 task
@danyx23 danyx23 force-pushed the data-page-bake-on-all-eligible-charts-from-etl branch from 2b18987 to 0ff197c Compare October 31, 2023 15:56
@danyx23 danyx23 changed the base branch from master to 10-31-data-page-wider-chart October 31, 2023 17:48
@danyx23 danyx23 force-pushed the data-page-bake-on-all-eligible-charts-from-etl branch from 0ff197c to b8fea76 Compare October 31, 2023 17:48
This was referenced Oct 31, 2023
Merged
Closed
@danyx23 danyx23 changed the title ✨ enable rendering of data pages on all eligible data pages, remove old datapage ✨ Data pages: enable rendering of data pages on all eligible data pages, remove old datapage Nov 2, 2023
@danyx23 danyx23 marked this pull request as ready for review November 2, 2023 09:08
This was referenced Nov 3, 2023
Merged
Merged
@sophiamersmann sophiamersmann force-pushed the 10-31-data-page-wider-chart branch from 56d05e8 to 807e17d Compare November 3, 2023 11:15
@sophiamersmann sophiamersmann force-pushed the data-page-bake-on-all-eligible-charts-from-etl branch from b8fea76 to 69a0883 Compare November 3, 2023 11:15
sophiamersmann
sophiamersmann approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@sophiamersmann sophiamersmann left a comment

Choose a reason for hiding this comment

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

Looks good to me!

If it's easy to do, should we rename DataPageV2 to DataPage?

baker/GrapherBaker.tsx
}
// If we have a single Y variable and that one has a schema version >= 2,
// meaning it has the metadata to render a datapage, then we show the datapage
// based on this information. Otherwise we see if there is a legacy datapage.json
Copy link
Member

Choose a reason for hiding this comment

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

Support for legacy datapage.json configs is dropped, no?

Would also be good to update the code comment above 👆

baker/GrapherBaker.tsx
return await renderDataPageV2({
variableId,
variableMetadata,
isPreviewing: true,
Copy link
Member

@sophiamersmann sophiamersmann Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is isPreviewing=true right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the code as is works as intended - this is in the function that shows the preview so we do want to set isPreviewing to true here

@danyx23 danyx23 force-pushed the 10-31-data-page-wider-chart branch from 807e17d to 6d8259d Compare November 8, 2023 09:39
@danyx23 danyx23 force-pushed the data-page-bake-on-all-eligible-charts-from-etl branch from 69a0883 to b35fc80 Compare November 8, 2023 09:39
@sophiamersmann sophiamersmann force-pushed the 10-31-data-page-wider-chart branch from 6d8259d to d621e18 Compare November 8, 2023 10:36
@sophiamersmann sophiamersmann force-pushed the data-page-bake-on-all-eligible-charts-from-etl branch from b35fc80 to 8104801 Compare November 8, 2023 10:36
This was referenced Nov 8, 2023
Merged
Merged
This was referenced Nov 12, 2023
Merged
Merged
@sophiamersmann sophiamersmann force-pushed the 10-31-data-page-wider-chart branch from d621e18 to 83c169b Compare November 16, 2023 16:05
@sophiamersmann sophiamersmann force-pushed the data-page-bake-on-all-eligible-charts-from-etl branch from 8104801 to fb3c4d8 Compare November 16, 2023 16:05
@danyx23 danyx23 mentioned this pull request Nov 16, 2023
Merged
@danyx23 danyx23 force-pushed the 10-31-data-page-wider-chart branch from 83c169b to 35f763f Compare November 17, 2023 15:54
@danyx23 danyx23 force-pushed the data-page-bake-on-all-eligible-charts-from-etl branch from fb3c4d8 to b7a6976 Compare November 17, 2023 15:54
This was referenced Nov 17, 2023
Merged
Merged
Merged
@sophiamersmann sophiamersmann force-pushed the data-page-bake-on-all-eligible-charts-from-etl branch 3 times, most recently from 66ecb88 to dd9778f Compare November 24, 2023 09:07
@danyx23 danyx23 force-pushed the 10-31-data-page-wider-chart branch from 485f87a to 68b16a6 Compare November 24, 2023 16:50
@danyx23 danyx23 force-pushed the data-page-bake-on-all-eligible-charts-from-etl branch from dd9778f to ad1ddab Compare November 24, 2023 16:50
@danyx23 Graphite App
Copy link
Contributor Author

danyx23 commented Nov 27, 2023 •
edited
Loading

Merge activity

  • Nov 27, 6:24 AM: @danyx23 started a stack merge that includes this pull request via Graphite.
  • Nov 27, 6:25 AM: @danyx23 merged this pull request with Graphite.

Base automatically changed from 10-31-data-page-wider-chart to master November 27, 2023 11:24
@danyx23 danyx23 merged commit c9619c6 into master Nov 27, 2023
12 checks passed
@danyx23 danyx23 deleted the data-page-bake-on-all-eligible-charts-from-etl branch November 27, 2023 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sophiamersmann sophiamersmann sophiamersmann approved these changes

Assignees

@danyx23 danyx23

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danyx23 @sophiamersmann