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.

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

Update plots types to accomodate static plot data #998

Merged
merged 16 commits into from
Nov 5, 2021

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Nov 4, 2021

4/5 master <- #1001 <- #995 <- #996 <- this <- #999

This PR updates the PlotsData type to accommodate static plots being sent to the webview.

@mattseddon mattseddon changed the base branch from master to add-plots-data-management November 4, 2021 06:08
@mattseddon mattseddon force-pushed the add-plots-data-management branch from 89f750b to a0b2cee Compare November 4, 2021 22:09
@mattseddon mattseddon force-pushed the update-types-for-new-data branch from db0de62 to 1097b0e Compare November 5, 2021 00:21
return this.experiments?.getLivePlots() || []
return {
live: this.experiments?.getLivePlots() || [],
static: this.staticPlots
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] We are going to have to rethink this if we don't want to hold a copy of the static plots data in the extension.

@mattseddon mattseddon force-pushed the add-plots-data-management branch from a0b2cee to 40cd0f0 Compare November 5, 2021 00:45
@mattseddon mattseddon force-pushed the update-types-for-new-data branch from 4d0b70a to 86e6918 Compare November 5, 2021 00:46
@mattseddon mattseddon marked this pull request as ready for review November 5, 2021 01:03
Array.isArray(data) &&
(data.length === 0 || (data[0].title && data[0].values))
)
return !!(hasKey(data, 'live') && hasKey(data, 'static'))
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] Deferring this decision until we have more information as to what a valid state for the plots webview is.

@@ -113,7 +113,7 @@ const Plot = ({
const Plots = ({ plotsData }: { plotsData?: PlotsData }) => {
return (
<>
{plotsData?.map(plotData => (
{plotsData?.live?.map(plotData => (
Copy link
Member Author

Choose a reason for hiding this comment

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

Will conflict with #1000

Base automatically changed from add-plots-data-management to master November 5, 2021 19:08
@codeclimate
Copy link

codeclimate bot commented Nov 5, 2021

Code Climate has analyzed commit d7448f6 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.3% (0.0% change).

View more on Code Climate.

@rogermparent rogermparent merged commit b2bf4be into master Nov 5, 2021
@rogermparent rogermparent deleted the update-types-for-new-data branch November 5, 2021 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants