-
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
Selecting Experiments redefining #1924
Comments
Hey folks! Please share with me your comments. I think this is something that will help us to have a bit better user interaction experience. WDYT? |
@wolmir what will be happening with expS, when the new branch has been created? Please help me to understand. Thanks |
While I was trying to create a new branch in order to answer Max the command failed because the branch name was already taken. |
What happened here was that DVC created a new branch from that experiment, but nothing happened visually, except for that Toast message. Is there a need to see them in the table as well? |
@maxagin I liked the checkbox design! What should happen if I select an experiment with child checkpoints? |
And also: We already have an experiment toggling feature. |
|
In terms of the logic, it is the same. However please review the specs and let me know if there is something that functioning differently from what we currently have. |
All the above questions are resolved. Thank you @wolmir 👍 |
Context menu behaviour. @wolmir just for information. (! however, in VSCode it seems like always on the right side :) Screen.Recording.2022-06-20.at.2.23.44.PM.mov |
@maxagin Can you add a spec on Figma for an experiment with checkpoints, please? |
@wolmir yeah. working on it right now |
Here we are @wolmir . Take a look and let me know if I have missed something or not. |
Thank you! |
@maxagin It seems that it should be possible to star a child (checkpoint). There are also actions (drop down menu) that could be applied to those also. How will they interact with checkboxes? |
Good. Will create an example
I and @wolmir had met and I understood that there are no actions for the checkpoint currently. Let's please clarify this. |
@shcheklein At the moment the only option we can apply to multiple rows is to remove them. And this remove option is restricted to the checkpoint tips only. Should we add the checkbox to the checkpoints anyway? |
We can add it when we have a case. The point of this question was to clarify that currently, we do not have actions for the checkpoints. |
No, no need for this. |
In terms of selecting the rows with a checkbox, all the following platforms are works the same: airtable, wandb and studio. Exactly as proposed here. I completely agree with Matt that we better use a native set of tools where it is possible, but to my opinion, this and other similar cases are different as they have multiple actions at the element (the row) and user experience, in this case, dictates a different approach I think. Screen.Recording.2022-06-24.at.1.30.45.AM.movScreen.Recording.2022-06-24.at.1.31.56.AM.movScreen.Recording.2022-06-24.at.1.34.27.AM.mov |
In our example, the atomic record is the row. Clicking on the row means that you can interact with that row through the context menu. This should not break/interfere with the other ways that the user can interact with the row. I don't see the benefit of doing this through add an additional checkbox. It seems like the main reason for making this change is so we can drop the inline copy button and move to "the user will select and copy the value", this is fine in principle but we do not always have the space to show the full value. Meaning users can/will lose precision when extracting values from the table.
I disagree that the checkbox in Studio behaves the same way as what is proposed here. The checkbox in Studio is used for:
They are not used to select rows to interact with through a context menu. There is in fact no context menu available in Studio:
The Jupyter extension has a data viewer. Which looks like it uses something similar to the Webview UI toolkit's data-grid component. I've had a quick play with this and it seems very basic. There are cell-level interactions but not much else: My questions are
|
We want to be able to work with row:
We want to do this without contrast hover styles and unnecessary on-click styles. Regarding select on click. ... Would like to refer to this comment: #1924 (comment)
#1924 (comment) |
So, the key here "In our example, the atomic record is the row. " vs "cell is the atomic record". To be more constructive we need a better list of workflows we expect users to do with cells and if those can be replaced with anything
anything else? My intuition was:
It's somewhat related, but we can actually keep (and probably should) the button and reconsider this later. No matter we do checkboxes or not.
Discrepancy between using circles and checkboxes for plots bother me as well though. My take - both of them are simpler to what we are trying to achieve here. (comes down to the level of granularity again that we need to agree on)
Any specific suggestions in my mind? Do you mean, btw, "actions" (like drop the whole concept of stars, or copy-paste cell, or don't allow multi-select at all, etc?) |
Experiments are immutable once they are finished the only row that we would be able to inline edit is the workspace record. As the workspace record is the only record that could have inline edit and it will probably never have any multi-select options we should consider disabling "selection" of this row altogether.
In order to star multiple rows I would have to click on the checkboxes (right next to the star), select the ones that I want and then invoke the context menu to star the records.... why not just click on the star icons?
The existing button works well for this. Mainly because we do not display all of the decimal places for numeric values. See below for an example of where we would lose precision by selecting and copying the value.
They should be able to click anywhere on the row (that isn't a copy button). What are the proposed cell-level actions outside of copy that make sense? Having the tooltip activate only through a hover makes sense to me.
I don't see anything outside of these options. I would not add the show tooltip on click option as this is also not available anywhere else in VS Code.
Drop stars, drop checkboxes, focus around the dot for selection. Leave multi-select how it is implemented now. Give the option to collapse to selected but have it based on dots. This would replace the need for stars altogether. This aligns with the behavior in Studio of selecting up to 7 commits to display together or in trends (plots). Being able to "favourite" more than 7 experiments seems unnecessary and adds clutter to the table. This also gets over the issue of not being able to display stars in the tree. |
That alone can annoy, wdyt?
More or less, but potentially - click can activate (and pin) tooltips. That's what Studio does because it has more information and we will have too.
It's available to some extent for the notifications. Also, we can't completely limit ourselves to the behavior available in VS Code. E.g. recently we were expanding tooltips to add links to them - what would be a solution there if tooltips in VS Code disappear immediately. For files we'll want to show more information in those tooltips, we want people to allow to copy all of it or parts of it.
Studio doesn't have stars semantics and anything close to that yet. To be honest, I don't quite understand how "up to 7 commits" and "basing these on dots" can solve the problem that stars are solving. Dots are used not to "favorite" something but as a replacement for an "eye" icon - to show / hide on the plots dashboard. Stars have a completely different semantics, right? I can star something (and definitely 7+ experiments), but I don't see all of them on the plots.
Most likely we'll have to write our custom tree if needed to accommodate our needs. Again, my feeling is that we go into extremes sometimes (both ends) - let's do a complete SaaS in VS Code and let's not use anything at all that doesn't exist in VS Code. |
I just tried to propose a more clear solution for the user, also taking into consideration the platform specifics :) Thanks |
@shcheklein @mattseddon @maxagin Should I hold the stars PR for now too? Until we can discuss this further, possibly in the planning today? |
I think we agreed on doing stars separately a while ago. I see that they are almost ready. I would go forward and merge them. For now I don't see any better suggestion that could replace them and it's only somewhat related to the things discussed here. |
@maxagin @mattseddon @shcheklein @rogermparent Screen.Recording.2022-06-30.at.16.13.21.mov |
thanks @wolmir - that looks cool to me |
With shift+click batch selection: Screen.Recording.2022-06-30.at.17.48.50.mov |
Hey @wolmir. Here are a few comments I have collected during my latest review of the exps table:
|
Thanks @maxagin I'm implementing a new demo for today's planning meeting with this UX in mind |
@mattseddon @maxagin @shcheklein What's the correct behavior for bulk selection?
|
@wolmir for now, it is: check the checkbox -> Shift (Mac) -> check another checkbox - will select all in between. Or I am missing something? Are you working on the bulk selection? |
It's because of this particular comment from Matt: #1964 (comment) I already fixed the bulk selection bugs, but I want to implement the correct behavior for that case. |
Good! Have I answered your question here #1924 (comment) Thank you @wolmir |
I don't see how selecting every checkpoint in between would be useful, though. We can only star multiple selections for now |
Scenario: I want to star a few expS -> I have selected multiple rows -> I change my mind and need an option to unselect all. Right? |
@maxagin Sorry, I was talking about the multiple selection of rows See this demo here: #1986 (comment) As you can see, every checkpoint is selected in the range specified by the user. But the only option the user has at that point is to star/unstar them. So it seems more useful to select only the tips, because then the remove option is available as well. |
@wolmir Currently (on prod) it is working more clearly I think - if the parent is selected, the children are not. Screen.Recording.2022-07-05.at.2.55.34.PM.mov |
And unselect all |
Got it, thanks! |
Is the description in this ticket stale? |
@iterative/vs-code Given the description that remained, I believe it's safe to consider this done, because the objective was accomplished (moving from line-clicks to checkboxes). The rest are improvements/fixes (created in #2028 ) |
This is an attempt to improve the interaction that is currently in place, also in tight relation with other ongoing work. The main motivation is to provide the user ability (to interact) to work with the row content (for example modifying the cell's info directly in the experiments table) without seeing the unnecessary styles. Any click on the row in the current case will trigger the BG style to appear, this is what we want to resolve.
Full information can be found in Figma.
TODO
The text was updated successfully, but these errors were encountered: