-
Notifications
You must be signed in to change notification settings - Fork 28
Add title option to plot wizard #4786
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
Conversation
* simplify plot object * simplify resourcePicker
* fix typo * use same keys message for multi and single files
Not sure why the test coverage is so low, everything seems to be tested for unless I'm missing something 🤔 |
extension/src/pipeline/quickPick.ts
Outdated
const template = await quickPickOne(PLOT_TEMPLATES, 'Pick a Plot Template') | ||
|
||
if (!template) { | ||
return | ||
} | ||
|
||
const title = await getInput(Title.ENTER_PLOT_TITLE, `${template}_plot`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[C] If you move this to after the selection of files/variables then we could provide a more meaningful default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e ${x} vs ${y}
or ${filename}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[C] If you move this to after the selection of files/variables then we could provide a more meaningful default.
i.e ${x} vs ${y} or ${filename}.
We could do that, but wouldn't we have to add extra checks if the user has selected multiple files or variables? Seems simpler to just keep a basic default, but if you think it would be valuable to have the name be more meaningful, I can update the code to use an updated title if the user has only chosen two metrics 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO ${x[0]} vs ${y[0]}
would still be a better default and not too hard to do. ${template}_plot
really isn't meaningful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative would be to not provide a default and let the user skip the title step by entering empty string. Would need to make this clear in the UI. Could probably do this by adding placeholder text instead of a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! I'll use ${x[0]} vs ${y[0]}
as a default.
Code Climate has analyzed commit 9598ee7 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 89.4% (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. |
1/2
main
<- this <- #4787Demo
https://github.com/iterative/vscode-dvc/assets/43496356/fba2492e-c6e4-4e85-bd1b-2b0da36ecda0Screen.Recording.2023-10-10.at.7.52.43.AM.mov
Part of #4654