Skip to content

Conversation

@julieg18
Copy link
Contributor

@julieg18 julieg18 commented Aug 29, 2023

1/3 main <= this <= #4627 <= #4628

Demo

Screen.Recording.2023-09-06.at.6.30.47.PM.mov

Fixes #3539

@julieg18 julieg18 added the product PR that affects product label Aug 29, 2023
@julieg18 julieg18 self-assigned this Aug 29, 2023
@julieg18 julieg18 changed the title Add UI for top-level plots creation Add top-level plots wizard Sep 6, 2023
const plotYaml = yaml.stringify({ plots: [{ [plotName]: plot }] }).split('\n')

// TBD this only works correctly for yaml with 2 space indent and no plots
// will adjust for other possibilities in another pr
Copy link
Contributor Author

@julieg18 julieg18 Sep 7, 2023

Choose a reason for hiding this comment

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

Will get taken care of in #4628 (Almost done, found a bug while finishing up tests. Will finish it tomorrow).

Copy link
Contributor

Choose a reason for hiding this comment

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

[C] Let's not leave comments TODO in the code/merge before fixing obvious/known issue. It would have been enough to add a do not merge label to this PR and leave a comment linking to #4628 instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! Removed the unneeded comments.

export const pickPlotConfiguration = async (): Promise<
PlotConfigData | undefined
> => {
// TBD data file validation will be in next pr
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taken care of in #4627.

"icon": "$(symbol-class)"
},
{
"title": "Add Plot",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe "Add Top-Level Plot" to better differentiate between "Add Custom Plot"?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is ok for now.

},
{
"command": "dvc.addTopLevelPlot",
"when": "dvc.commands.available && dvc.project.available"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is currently available in the command pallette.

@julieg18 julieg18 marked this pull request as ready for review September 8, 2023 00:00
Copy link
Contributor

@mattseddon mattseddon left a comment

Choose a reason for hiding this comment

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

👍🏻

const plotYaml = yaml.stringify({ plots: [{ [plotName]: plot }] }).split('\n')

// TBD this only works correctly for yaml with 2 space indent and no plots
// will adjust for other possibilities in another pr
Copy link
Contributor

Choose a reason for hiding this comment

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

[C] Let's not leave comments TODO in the code/merge before fixing obvious/known issue. It would have been enough to add a do not merge label to this PR and leave a comment linking to #4628 instead.

return
}

return { ...templateAndFields, dataFile: file }

Choose a reason for hiding this comment

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

Avoid too many return statements within this function.

@julieg18 julieg18 enabled auto-merge (squash) September 11, 2023 17:10
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 0a0960f and detected 0 issues on this pull request.

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

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

View more on Code Climate.

@julieg18 julieg18 merged commit 8842368 into main Sep 11, 2023
@julieg18 julieg18 deleted the add-plots-wizard branch September 11, 2023 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

product PR that affects product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extend/Reuse Custom Plots for building Top-Level plots

2 participants