Select the number of items per row from slider #3405

sroy3 · 2023-03-06T15:59:01Z

Part of #2585

Demo

Screen.Recording.2023-03-07.at.1.45.23.PM.mov

Demo on a bigger screen (higher maximum number of plots per row)

Screen.Recording.2023-03-07.at.1.47.07.PM.mov

Scope

Remove the resizer on each plot and add a slider to select the number of plots per row
Slider is only available if there are plots in the section
Slider is not available in the comparison table (still no resize in that section)
Maximum number of plots per row is dynamic. It changes with the screen size. Minimum width of one plot is 300px

sroy3 · 2023-03-06T16:06:55Z

Just used the current controls we had, but we could probably change this for a slider. We can either add more options or add a custom option at the end for the user to specify what they want. In the end, it does simplify things by a lot, it'll be way easier to maintain, and will definitely improve performance.

shcheklein · 2023-03-06T18:19:29Z

Thanks, @sroy3 !

Let's do a few changes (going forward, I know this is just a POC):

Add a very explicit message before the icon that this is about plots layout
Let's not limit it by 4? I'm not sure why we have this limitation tbh.
Q: how does it affect the large plots?

sroy3 · 2023-03-06T18:37:55Z

Thanks, @sroy3 !

Let's do a few changes (going forward, I know this is just a POC):

Add a very explicit message before the icon that this is about plots layout

There are tooltips that pop pretty much instantly on these icon buttons. All plots controls are centralized there. Discovery shouldn't be hard. If we add a message, we'd have to rethink that whole menu. We have more important actions without obvious labels either. As a matter of fact, most of the actions in the extension are only one icon and a tooltip. We can try to find better icons as well if necessary.

Screen.Recording.2023-03-06.at.1.33.35.PM.mov

Let's not limit it by 4? I'm not sure why we have this limitation tbh.

I can do that easily (more than before). I think we had limited to 4 because on a small screen it starts being quite small and calculating break points becomes more difficult the smaller the screen is (but we do not have those break points anymore, everything is done by css). We should limit this to how much then? 10? Should we just have a numerical input instead? By the way should these icon buttons simply open a quick pick / quick input like the buttons for custom plots? We currently have 2 strategies here, custom controls and using quick pick / quick inputs. I'll generalize them at the same time (use the quick picks / inputs).

Q: how does it affect the large plots?

It doesn't for now. But we could maybe do some calculations to make it fit. Something like, if there are 4 items per rows and the large plot has 2 revisions in it, then place 2 plots on the row. But if there are 3 items per row or 4+ revisions, stick to 1 plot per row. This will get better if we increase the possible number of items per row.

shcheklein · 2023-03-06T18:56:05Z

There are tooltips that pop pretty much instantly on these icon buttons.

Yep, but I think it's not enough unfortunately. With resize the issue is that I (as a user) expect it to be naturally part of the plots (e.g. grip, an icon on the plot, etc). It's less natural and no single standard way to make it part of the menu. Thus I would advocate for explicit controls (text + something) vs buttons only. We can always optimize it back, but there is no reason for us to make it harder for people to find.

But we could maybe do some calculations to make it fit.

Yep, sounds good (there are some details still to clarify, but initial approach sounds reasonable to me).

sroy3 · 2023-03-07T18:53:00Z

Demo was updated

mattseddon

👍🏻

mattseddon · 2023-03-08T02:44:40Z

webview/src/shared/components/slider/styles.module.scss

+  color: $fg-color;
+}
+
+.slider {


Might be a good time to revisit the in-plot sliders and match them with the background?

Not sure I get what you mean, aren't both slider the same?

The background color of the slider in the plots could take the webview background color ($bg-color) instead of $plot-block-bg-color.

https://github.com/iterative/vscode-dvc/blob/main/webview/src/plots/components/styles.module.scss#L51

Just a suggestion.

I see. I'll do that as a quick follow-up, no problem

mattseddon · 2023-03-08T02:46:02Z

extension/src/plots/webview/contract.ts

-  FOUR: 4
-}
-/* eslint-enable sort-keys-fix/sort-keys-fix */
+export const DefaultNumberOfItemsPerRow = 2


[Q] Should this be calculated based on the screen size?

Not sure myself, just asking the question.

[N] Should use screaming snake case. e.g NUM_OF_COMMITS_TO_SHOW

[N] Should use screaming snake case. e.g NUM_OF_COMMITS_TO_SHOW

I got confused and was wondering about this because of other variables not using this format. Will change that.

[Q] Should this be calculated based on the screen size?

Good question. I could try this as a follow up. Currently the default vale is set in the extension, so I'd have to reset everything to undefined to then calculate it in the webview. I'd probably select the middle value by default.

extension/src/plots/vega/util.ts

webview/src/plots/components/App.test.tsx

mattseddon · 2023-03-08T03:00:11Z

webview/src/plots/components/checkpointPlots/CheckpointPlotsWrapper.tsx

@@ -42,6 +44,8 @@ export const CheckpointPlotsWrapper: React.FC = () => {
      menu={menu}
      nbItemsPerRow={nbItemsPerRow}
      sectionCollapsed={isCollapsed}
+      changeNbItemsPerRow={changeSize}


[C] I just realised that updating both Custom and Trend plots is going to collide with #3404

cc @julieg18

Yes, there, for sure, will be some collisions. Though I don't think they'll be anything too hard to correct :)

webview/src/plots/components/styles.module.scss

webview/src/plots/components/Plots.tsx

webview/src/plots/components/PlotsContainer.tsx

webview/src/plots/components/webviewSlice.ts

julieg18

Great work!

webview/src/shared/components/slider/MinMaxSlider.tsx

julieg18 · 2023-03-08T14:28:45Z

webview/src/shared/components/slider/MinMaxSlider.tsx

+  onChange: (newValue: number) => void
+}
+
+export const MinMaxSlider: React.FC<MinMaxSliderProps> = ({


Not sure if there is anything that can be done about it but the slider jumps sometimes when the plot rows increase:

Screen.Recording.2023-03-08.at.8.27.14.AM.mov

The way to solving that would be to make sure the last section is at least 100vh which does not always look good, especially when it's empty or have very few plots. I'm wondering though if we shouldn't have one width slider for all plots instead of resizing per section.

webview/src/shared/components/slider/MinMaxSlider.tsx

sroy3 · 2023-03-08T16:01:33Z

Newest demo:

Screen.Recording.2023-03-08.at.10.27.54.AM.mov

codeclimate · 2023-03-08T16:10:59Z

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

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

This pull request will bring the total coverage in the repository to 95.7%.

View more on Code Climate.

Stephanie Roy added 3 commits March 6, 2023 09:41

Added a story to visualize the icons

dd0100b

Disable chromatic snapshots

5cbbe7c

Select the number of items per row from picker

d842ea3

sroy3 added the product PR that affects product label Mar 6, 2023

sroy3 self-assigned this Mar 6, 2023

sroy3 changed the base branch from main to icons-story March 6, 2023 16:00

sroy3 mentioned this pull request Mar 6, 2023

Plots grid with plot resize #2585

Closed

10 tasks

Use const values

5b79ecf

sroy3 mentioned this pull request Mar 6, 2023

Change the aspect ratio of plots through picker #3407

Closed

Base automatically changed from icons-story to main March 6, 2023 19:55

Stephanie Roy added 2 commits March 7, 2023 11:38

Use slider instead

49ec3e4

Resize multiview plots

2a5f87e

sroy3 changed the title ~~Select the number of items per row from picker~~ Select the number of items per row from slider Mar 7, 2023

Stephanie Roy and others added 4 commits March 7, 2023 14:04

Self review

1b4a76a

Merge branch 'main' into items-per-row-control

38d3231

Fix and add tests

309f78d

Merge branch 'main' into items-per-row-control

28bf685

sroy3 marked this pull request as ready for review March 7, 2023 21:21

sroy3 requested review from mattseddon and julieg18 as code owners March 7, 2023 21:21

sroy3 requested a review from shcheklein March 7, 2023 21:25

mattseddon approved these changes Mar 8, 2023

View reviewed changes

julieg18 approved these changes Mar 8, 2023

View reviewed changes

Apply review comments

fbc5d26

Merge branch 'main' into items-per-row-control

4990930

sroy3 merged commit b48c80d into main Mar 8, 2023

sroy3 deleted the items-per-row-control branch March 8, 2023 16:45

sroy3 mentioned this pull request Mar 8, 2023

Change the background color for the smooth slider over plots #3427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select the number of items per row from slider #3405

Select the number of items per row from slider #3405

sroy3 commented Mar 6, 2023 •

edited

Loading

sroy3 commented Mar 6, 2023 •

edited

Loading

shcheklein commented Mar 6, 2023

sroy3 commented Mar 6, 2023 •

edited

Loading

shcheklein commented Mar 6, 2023

sroy3 commented Mar 7, 2023

mattseddon left a comment

mattseddon Mar 8, 2023

sroy3 Mar 8, 2023

mattseddon Mar 8, 2023 •

edited

Loading

sroy3 Mar 8, 2023

mattseddon Mar 8, 2023

mattseddon Mar 8, 2023

mattseddon Mar 8, 2023

sroy3 Mar 8, 2023

sroy3 Mar 8, 2023

mattseddon Mar 8, 2023

julieg18 Mar 8, 2023

julieg18 left a comment

julieg18 Mar 8, 2023

sroy3 Mar 8, 2023

sroy3 commented Mar 8, 2023

codeclimate bot commented Mar 8, 2023

Select the number of items per row from slider #3405

Select the number of items per row from slider #3405

Conversation

sroy3 commented Mar 6, 2023 • edited Loading

Demo

Demo on a bigger screen (higher maximum number of plots per row)

Scope

sroy3 commented Mar 6, 2023 • edited Loading

shcheklein commented Mar 6, 2023

sroy3 commented Mar 6, 2023 • edited Loading

shcheklein commented Mar 6, 2023

sroy3 commented Mar 7, 2023

mattseddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattseddon Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroy3 commented Mar 8, 2023

codeclimate bot commented Mar 8, 2023

sroy3 commented Mar 6, 2023 •

edited

Loading

sroy3 commented Mar 6, 2023 •

edited

Loading

sroy3 commented Mar 6, 2023 •

edited

Loading

mattseddon Mar 8, 2023 •

edited

Loading