-
Notifications
You must be signed in to change notification settings - Fork 29
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
plots: copy edit walkthrough and dashboard tooltips #1922
Conversation
Should I feel free to merge (my) approved PRs in this repo? Thanks |
@jorgeorpinel I'll review it, haven't had time yet |
sections for selected experiments, that correspond to the different | ||
[types of plots](https://dvc.org/doc/command-reference/plots#supported-file-formats) | ||
supported by DVC: | ||
If you're using Python, the [DVCLive] helper library can save plots data for |
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.
You are adding too much context to the page, too much structure. It tried as much as I can to keep it in a way that at least on my 13'' screen I can see some actual plots screenshots when open it
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
too much structure
Are you referring to the H2s further down? Also not a strong opinion, I guess it's a stylistic choice. IMO it looks better and clearer (# Plots > ## Plot types
) and again it doesn't make the page longer. But I'm happy to flatten it if you confirm that's what we're talking about.
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.
It's not about the page length per se. It's about when I can see any plots on the page that is called "Plots". We want the screenshot with them visible asap. Adding more paragraphs, structure, etc are making it more and more like a tutorial. We should be throwing out things if possible, not adding them. It's similar to DVC's get started - we are fighting for every symbol here.
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.
We should be throwing out things if possible... we are fighting for every symbol
OK, I compressed it again, in fact I merged a few things to make it even shorter. PTAL
It's about when I can see any plots on the page that is called "Plots"
Maybe we should move the 💡 tip about setting up plots to the bottom then? Or just link to DVC docs
[Section.CHECKPOINT_PLOTS]: | ||
'Linear plots based on data from the experiments table.', | ||
'Real-time graphs based on metrics from the Experiments Table', |
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.
Let's create a ticket please to being able to include links into these descriptions.
- keep all three sections visible, if section is empty, show the same message instead of it's content
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.
Ticket created: #1933
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.
Do you also think "graphs" is also not good here? I like it better here since trends charts are not regular DVC plots but rather an extension-specific feature I think. Or does DVCLive also update regular plots in your browser? Anyway, feel free to also commit this if you prefer so:
'Real-time graphs based on metrics from the Experiments Table', | |
'Real-time plots based on metrics from the Experiments Table', |
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.
yes. Also not sure about making it the "Experiments Table" tbh.
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.
It was already "Experiments Table". That's the official name we give to that custom VS Code Editor I think?
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.
sorry, I meant capitalization - should it be "Experiments Table" or just "experiments table" in these descriptions?
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 like capitalization and bolding since each one has a walkthrough page. No strong opinion
BTW this page should be Plots Dashboard, I think (bit stronger opinion there).
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.
okay, let's keep it
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.
Another 2 places where we have "graph" at the moment. Up to you guys, thanks:
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
…-dvc into editors/plots-dashboard
[**Plots Dashboard**](command:dvc.showPlots). Use | ||
[`DVC: Show Plots`](command:workbench.action.quickOpen?%22>DVC:%20Show%20Plots%22) |
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.
Do we need both links? I think they do the same thing in the end.
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.
it educates ppl a bit more
Code Climate has analyzed commit 5f4d273 and detected 0 issues on this pull request. 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.6% (0.0% change). View more on Code Climate. |
No description provided.