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

Plots grid with plot resize #2585

Closed
10 tasks done
sroy3 opened this issue Oct 13, 2022 · 38 comments
Closed
10 tasks done

Plots grid with plot resize #2585

sroy3 opened this issue Oct 13, 2022 · 38 comments
Assignees
Labels
A: plots Area: plots webview, side panel and everything related priority-p1 Regular product backlog

Comments

@sroy3
Copy link
Contributor

sroy3 commented Oct 13, 2022

We want to redefine the plots grid and allow resizing of the plots (proposal can be found here).

We can split this in a couple of steps:

  • Change the internal structure to use numbers instead of the defined PlotSize enum. (Make the plot sizes use numbers underneath #2563)
  • Allow resize of plot horizontally
  • Make the default height of plot 280px (since we have a 4:3 ratio for plots, that means 375px wide)
  • Remove the size picker
  • Fix resizing of last item(s)
  • Allow resize of plot vertically
  • Resize of comparison table
  • Add a button to reset the size
  • Make scrollbars local to a section
  • Make sliders sticky
  • Calculate the default number of items per row from the screen size
  • Make section headers sticky
@sroy3
Copy link
Contributor Author

sroy3 commented Oct 13, 2022

  • Fix resizing of last item(s)

If you're dragging the corner of the last element and it resizes at the same time, you'll soon find yourself running out of space to resize while if you were resizing the first element instead, the other elements would just start moving to the next line.

@mattseddon proposed using a slider instead of dragging a corner of the plot to fix this issue. This way we can quickly see how the resize is affecting all plots, we could set a clear minimum and maximum, and steps would also be easier to get right.

@sroy3 sroy3 self-assigned this Oct 13, 2022
@sroy3 sroy3 added priority-p1 Regular product backlog A: plots Area: plots webview, side panel and everything related labels Oct 13, 2022
@shcheklein
Copy link
Member

@mattseddon proposed using a slider instead of dragging a corner of the plot to fix this issue. This way we can quickly see how the resize is affecting all plots, we could set a clear minimum and maximum, and steps would also be easier to get right.

we would need to keep a slider nearby each plot then though?

I feel it's still better to have a regular resize grip tbh. It's a thing that is common and expected vs something that is custom.

@sroy3
Copy link
Contributor Author

sroy3 commented Oct 13, 2022

@mattseddon proposed using a slider instead of dragging a corner of the plot to fix this issue. This way we can quickly see how the resize is affecting all plots, we could set a clear minimum and maximum, and steps would also be easier to get right.

we would need to keep a slider nearby each plot then though?

I feel it's still better to have a regular resize grip tbh. It's a thing that is common and expected vs something that is custom.

I'd keep it in the top bar (a resize icon button + slider dropdown).

I agree it feels more natural to grip and resize, but we have to keep in mind that there are multiple edge cases regarding this solution.

It does not even have to be the last item(s) from one line. Let say you have a grid that is currently 4x4. You resize the first one of the third row so that the grid is now 2X8. The third row is now the fifth one (the whole layout moved while you were resizing). If we decide to apply the resize after the grip, we still have a problem that we might not be able to stretch some elements as much as we want because we hit the end of the row or column.

@sroy3
Copy link
Contributor Author

sroy3 commented Oct 13, 2022

Another solution would be to have lines between the plots (low opacity, higher opacity on hover + resize cursor). It would resize more like a table instead of resizing plots like windows.

@shcheklein
Copy link
Member

Fair points @sroy3. Especially with the edges. May be that is fine for now that the last plot at edge will have limited resizing options of we can even disable it there ... I need to find some examples of this

(still have mixed feeling about the custom stuff, it's quite a sacrifice, it's fine to do actual resize after the fact)

@shcheklein
Copy link
Member

E.g. here https://wandb.ai/wandb/wandb_example?workspace=user- - it doesn't allow me to resize the last plot (except downsizing it).

@maxagin
Copy link
Contributor

maxagin commented Oct 14, 2022

Edge case: how to resize the last cell in a row? E.g. from 3 to 2 in line. Possible solution: when you start increasing the cell, show the outline in the second row, where the cell will be located after the resizing. Actually, it is the same principle we use everywhere - to show where and how the block will be located before changing the composition.

https://www.notion.so/iterative/Resize-responsiveness-grid-for-Plots-WebView-39c16e0e53a149f09ffdd23b33950ec8#01a96cf1c4a94f129901fc39934096ab

@sroy3
Copy link
Contributor Author

sroy3 commented Oct 14, 2022

E.g. here https://wandb.ai/wandb/wandb_example?workspace=user- - it doesn't allow me to resize the last plot (except downsizing it).

This works because the resize is done after, but it is easy to lose track of the item you were resizing when it suddenly drops down a couple of rows and moves a couple of columns.

@sroy3
Copy link
Contributor Author

sroy3 commented Oct 14, 2022

Edge case: how to resize the last cell in a row? E.g. from 3 to 2 in line. Possible solution: when you start increasing the cell, show the outline in the second row, where the cell will be located after the resizing. Actually, it is the same principle we use everywhere - to show where and how the block will be located before changing the composition.

https://www.notion.so/iterative/Resize-responsiveness-grid-for-Plots-WebView-39c16e0e53a149f09ffdd23b33950ec8#01a96cf1c4a94f129901fc39934096ab

But what if there is no more room to move your cursor right? I know you can always pick another plot to resize (and eventually only the first one would be used as it's the easiest to really resize), but what's the point of being able to resize every plot if we hit edge case with pretty much every one of them?

@maxagin
Copy link
Contributor

maxagin commented Oct 14, 2022

But what if there is no more room to move your cursor right?

There will be always some room on the sides.

@maxagin
Copy link
Contributor

maxagin commented Oct 14, 2022

I can see two options (this is already has been discussed, but I can not find it on the Notion. Probably this was published as a comment).

  1. We are letting to resize the last one
  2. Can not resize the last one

I would love if we can do it better than wandb, and make the last one also resizable :)

@sroy3
Copy link
Contributor Author

sroy3 commented Oct 14, 2022

I can see two options (this is already has been discussed, but I can not find it on the Notion. Probably this was published as a comment).

  1. We are letting to resize the last one
  2. Can not resize the last one

I would love if we can do it better than wandb, and make the last one also resizable :)

It's not just a problem with the last one, it's gets more problematic going diagonally down the grid. The one none edge case would be the first plot. After that, all plots have some kind of edge case associated with them.

Btw, the plan is still to implement resizing with a grip. I'm just surfacing the problems I see arising later down the road so that we have time to figure out solutions.

@shcheklein
Copy link
Member

I'm fine to do the slider initially. Considering how many edge cases we have with this UI/UX will be clunky (unless we have some ideas how to make it feel and look good). If we do slider there should be an explicit "Grid size: " element on the screen to control it. Per section. It might changes the logic a bit behind the resize, breaking points, etc. @maxagin pls take a look.

If it's easier (don't require too many extra hours) we can first implement grip to try it.

@maxagin
Copy link
Contributor

maxagin commented Oct 14, 2022

It's not just a problem with the last one, it's gets more problematic going diagonally down the grid

Just want to share an example. Please let me know wdyt

Screen.Recording.2022-10-14.at.2.58.45.PM.mov

@sroy3
Copy link
Contributor Author

sroy3 commented Oct 14, 2022

It's not just a problem with the last one, it's gets more problematic going diagonally down the grid

Just want to share an example. Please let me know wdyt

Screen.Recording.2022-10-14.at.2.58.45.PM.mov

These aren't really edge cases as there is room to drag. Also, do we really want to resize after? It makes it very hard to find the plot you're resizing after. It's hard with just a few plots, I can't imagine with hundreds of them.

@maxagin
Copy link
Contributor

maxagin commented Oct 14, 2022

These aren't really edge cases as there is room to drag.

Please list a few examples of what you are calling edge cases or all of the cases please.

@maxagin
Copy link
Contributor

maxagin commented Oct 14, 2022

If we do slider there should be an explicit "Grid size: " element on the screen to control it. Per section. It might changes the logic a bit behind the resize, breaking points, etc. @maxagin pls take a look.

Yes, of course, this will change the logic.

One of the things than comes to my mind right away is that in this case, we will have two controls - horizontal and vertical :)

@sroy3
Copy link
Contributor Author

sroy3 commented Oct 14, 2022

These aren't really edge cases as there is room to drag.

Please list a few examples of what you are calling edge cases or all of the cases please.

The extreme one that would include almost all edge cases: wanting to make the plots full width and doing so on any but the first element of a row. You're going to hit the edge soon enough.

Then do we resize while dragging or not? Both have their pros and cons. Like previously mentioned, resizing after dragging means having to find the plot you were trying to resize in this new layout. Resizing while dragging would help to follow the plot you are interested in, but the layout is also shifting. For both cases, it gets worse the more you go diagonally down the grid.

Since the elements are not independent (they all resize as one), I'm not sure grip and resize is the best solution here. I think a slider or resizing columns (maybe rows too) as a table would work with more fluidity and be simpler for us and the user.

@sroy3
Copy link
Contributor Author

sroy3 commented Oct 14, 2022

One of the things than comes to my mind right away is that in this case, we will have two controls - horizontal and vertical :)

Isn't there a ratio to plots or are we removing it? This is another case to think about (having enough room for content).

@maxagin
Copy link
Contributor

maxagin commented Oct 17, 2022

The extreme one that would include almost all edge cases: wanting to make the plots full width and doing so on any but the first element of a row. You're going to hit the edge soon enough.

If 4 in a row and you start increasing the first one. You will have 3 snapping points

Then do we resize while dragging or not?

The proposed way to do this is to resize only after. In the case of resizing right away, I do not see how the proposed in Notion behaviour will be helpful.

One of the things than comes to my mind right away is that in this case, we will have two controls - horizontal and vertical :)

Isn't there a ratio to plots or are we removing it? This is another case to think about (having enough room for content).

The width and height are controlled by the user ([example Notion](https://www.notion.so/iterative/Resize-responsiveness-grid-for-Plots-WebView-39c16e0e53a149f09ffdd23b33950ec8#e1c17a96cb6f44f582b232961592e0d7))

@sroy3
Copy link
Contributor Author

sroy3 commented Oct 19, 2022

Here is what resizing the grid instead of plots could look like:

Screen.Recording.2022-10-19.at.2.11.53.PM.mov

The idea is to grab a line and move it while the columns or rows resize (as one). There is only one direction at the time, but this would greatly stabilizes how we would handle the resize.

Here is how it could look with the accent color instead:

Screen.Recording.2022-10-19.at.2.21.54.PM.mov

It's easy to change the width, opacity or style (dashed, dotted...) of the line.

@shcheklein
Copy link
Member

I'm not sure about this from the UX perspective, but it's fine to implement and try it. I wonder if there are examples like this and how to make it clear to users what those lines mean.

Good thing about this is that we can probably even resize the grid dynamically and show the end result.

@sroy3
Copy link
Contributor Author

sroy3 commented Oct 21, 2022

I'm not sure about this from the UX perspective, but it's fine to implement and try it. I wonder if there are examples like this and how to make it clear to users what those lines mean.

Good thing about this is that we can probably even resize the grid dynamically and show the end result.

The cursor does help in knowing the purpose. I can probably make them more obvious as well. I'll focus on the feature for now and will improve on the style later.

@mattseddon mattseddon added the priority-p1 Regular product backlog label Feb 21, 2023
@sroy3
Copy link
Contributor Author

sroy3 commented Mar 3, 2023

So I've added vertical plot resizing (not complete yet, but ready to demo basic functionality). I'm not convinces about the result. IMO, I don't think it's a good idea to add as a feature and would even go as far as saying that horizontal resizing isn't ideal either.

Here is a quick video demo:

Screen.Recording.2023-03-03.at.2.52.27.PM.mov
  1. It would be very hard to resize multi view plots in that manner
  2. The aspect ratio can easily get skewed especially since we do not see the result until the resize is complete
  3. I often get surprised by the actual size of the plot after resize. The plot does not end where my cursor is
  4. When going from less to more items per row, it's difficult to understand what is going to happen as the dashed line involves removing width and maybe adding height
  5. Everything is worse when not resizing the first item as predicted
  6. Another solution would help avoid bugs and unexpected behaviour also making the code lighter

For resizing the width, we only have four options (1, 2, 3, or 4 items per row) let's simply show these options to the user up front.

For the height, maybe a few options with some ratios we know will look good.

These two new controls could easily be added to the icon menu.

@shcheklein @mattseddon @julieg18 I would appreaciate your thoughts on this. If we decide to continue with the current resize solution, I can fix the bugs next week. If we decide to change strategy, this could probably be done in the same amount of time it would take to fix the rest of the bugs. I'll open a draft PR with what I currently have if you want to look into it more deeply.

@shcheklein
Copy link
Member

Sounds good to (simpler option with controls). I'm not sure how would that work for the large plots, also not sure why it should be limited to 1, 2, 3, 4 tbh. I lost the context - but I though we expanded it to be > 4, and wanted to keep the aspect ratio, etc.

If we do controls - let's them make explicit: Number of plots per row: .... I'm not sure what to do with the horizontal size though.

Bottom line, your call on this. But it overall it sounds a bit complicated as well. (esp with multiple different controls for width and height).

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 6, 2023

I'll create a quick prototype to check this out today.

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 6, 2023

This is what it'd look like with a control to decide on the horizontal size: #3405
Took less than 30 minutes to build (just to show how much simpler that would make things).

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 6, 2023

Two different prototypes were created. One adding the resizing of plots vertically (#3393) and one using icon menu controls to change the horizontal size and the aspect ratio (#3405 and #3407). What would be the best path to continue working on the sizing of plots?

My vote goes to the second one. It's straight forward both UX-wise and code-wise. The user has some options, but they are well defined as opposed to the freestyle form that was previously used. If we decide to go with that option, here is what left:

  • Add sizing to multi view plot (easier to do when the control is on the whole section rather than on the plot itself).
  • Figure out how to resize the comparison table (we don't have the choice of number items per row and aspect ratio does will squash images)
  • Use an input for the number of items per row
  • Use quick picks / quick inputs instead of custom pickers (to match with custom plots actions). We will lose the possibility of quick feedback of the picker though, but things will be more uniform
  • Select better aspect ratios
  • Label controls explicitly. How about changing the icon menu to be a act differently? Let's add three vertical dots as it's often used for settings and add a dropdown menu of icons + labels. Would that be enough, or the fact that the options are hidden first would defeat the purpose? cc @shcheklein

We won't need a button to reset the size anymore with this strategy.

Lots of bugs needs fixing if we decide to continue with our current resize strategy, and that would be less maintainable, but it's not impossible to continue that going that route.

@shcheklein
Copy link
Member

Thanks @sroy3 for the summary. I'm fine to go with the second option as I mentioned in the ticket before.

or the fact that the options are hidden first would defeat the purpose?

Yes, I think it defeats the purpose.

How about having two sliders with labels instead of buttons? I think it would be the best way to control this tbh.

@mattseddon
Copy link
Member

I looked at the analytics for the last 30 days. There are 923 events for drag and drop of plots but only 68 resize events. Which tells me that it isn't getting used very often.

What is the simplest thing that we can do to improve this?

If we think sliders are the way to go then can we agree that under the hood it will work the same way as shown in #3407? The "smooth" control in the default DVC template also seems to have snap points for the slider so I think this would be acceptable.

With sliders there are a couple of questions that I think we need to answer:

  1. Where would they go? Could they go here:

image

  1. Would we need to make them sticky?
  2. There are no sliders in the @vscode/webview-ui-toolkit or the rest of VS Code so should we match the style shown in the smooth template?

Alternatively, if we use buttons could we tie that into the quick-pick menu as we do with custom plots:

Screen.Recording.2023-03-07.at.1.39.08.pm.mov

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 7, 2023

Sliders are great for the number of items per row. I can add this with a max of 10? For aspect ratio, it's much harder to visualize linearly, but that could still work. I will add sliders outside the icons menu where @mattseddon suggested, but again IMO this isn't the best way to go. Like @mattseddon said, the resize feature isn't deeply used and the default size given is quite reasonable. Why would we want to advertise this feature more than filtering, adding a new plot, queue experiments, sort, or all other actions that are displayed as a single icon and a tooltip? Having multiple places where the user can control the plots simply makes things more confusing. If it's actually the first thing the user does when opening the plots webview, I would suggest improving the default size first.

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 7, 2023

Slider looks good I think. It's fine to remove it from the icons menu (might be a good idea since icons in there should open quick picks / inputs and not custom controls).

Screen.Recording.2023-03-07.at.10.54.36.AM.mov

I think I'll make it so the maximum number of plots per row is defined by the screen size, because it makes no sense on some laptop screen to go above 4, while you might want more on a bigger screen. We had a 300px min width and that'll be used to calculate the max number of plots.

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 8, 2023

What is everyone's thought on having one single width slider for the whole plots webview instead of one for each plot section? That would solve this like this #3405 (comment)

@shcheklein
Copy link
Member

It limits user options clearly (the way I prefer see images can be very different from the way I see linear plots). If it's very hard to implement, let' do a single set of width / height controls, but we need to stick them, make them part of the ribbon then or something like that.

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 8, 2023

It limits user options clearly (the way I prefer see images can be very different from the way I see linear plots). If it's very hard to implement, let' do a single set of width / height controls, but we need to stick them, make them part of the ribbon then or something like that.

Other than images, I didn't see why someone would want template plots to be large and custom plots to be small at the same time, that's why I was proposing, but I totally fine either way. I don't think it would simplify having the sliders sticky, the only advantages to it is it would solve what is seen in the comment and might be less annoying to the user to resize once instead of twice.

@mattseddon
Copy link
Member

mattseddon commented Mar 8, 2023

Other than images, I didn't see why someone would want template plots to be large and custom plots to be small at the same time

In order to do this I think we would need to move images out of the middle (either to the top or bottom). Custom and Data Series would need to be grouped somehow.

edit:☝🏻 this was meant to be a question but didn't come out that way.

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 20, 2023

The "calculate the default number of items per row from the screen size" came from this comment. I tried to implement it in #3492, but I think it's not worth the trouble. It mainly has to do with how we truncate titles depending on screen size with Vega. We currently go through all the Vega plot data and look for titles on the extension side and truncate them if necessary. If we let the screen size decide the default number of items per row, we'd need to move this on the webview side and we'll loop over the data way more often. If the default number of items per row isn't right for the user, they can easily change it (that's what this whole ticket is about after all) and the new value will be persisted anyway.

@sroy3 sroy3 closed this as completed Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: plots Area: plots webview, side panel and everything related priority-p1 Regular product backlog
Projects
None yet
Development

No branches or pull requests

4 participants