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

Review: table styles #2241

Closed
27 tasks done
maxagin opened this issue Aug 23, 2022 · 30 comments · Fixed by #2384
Closed
27 tasks done

Review: table styles #2241

maxagin opened this issue Aug 23, 2022 · 30 comments · Fixed by #2384
Assignees
Labels
A: experiments Area: experiments table webview and everything related bug Something isn't working priority-p1 Regular product backlog 🔍 review A placeholder issue to review certain part of the product or story

Comments

@maxagin
Copy link
Contributor

maxagin commented Aug 23, 2022

I have reviewed the EXPs table and would like to share with you some comments.
I will be referring to this Figma file

TODO

  • Add tooltips to checkboxes and stars
  • Hide the right border of the hovered first cell after the horizontal scroll is activated and the shadow appears
  • Increase the workspace row height
  • Take off table header right closing line
  • Update on-hover header separator line background color
  • Take off the first cell's right border when the cell is sticky
  • Selected first cell does not have a shadow when scrolled horizontally
  • Keep checkpoint lines on top of the row border
  • Update the view background color
  • Inconsistent borders on head cells vs body cells
  • Remove sticky section bottom border after the vertical scroll is activated
  • Keep the resize icon and the vertical line (resize indicator) visible during column resize
  • Header cell resizer bar doesn't always reach the top of a cell
  • Remove the EXP name from being a toggle
  • When star an experiment the weird progress lines show on the sidebar. (Bug?)

Hint/Tooltip

  • Add hint "Add/Remove to plots" (maybe different wording) when the EXP's circle has hovered
  • Hints like Select, Add Star and similar. The text is too big. Try to make it the same as when you hover on the Experiments tab
  • Hints need to be closer to the cursor
  • Hints do not need to have a triangle at the top

Row

  • Update row selection styles
  • A number of hidden element indicators (circle with the number) styles for rested and selected row states
  • There is a little spacing between the elements in the design, for example, checkbox and star, it is important to show the user where the active area ends and where the new starts
  • Chevron on hover - cursor pointer
  • The table's last row does not need a button border
  • Checkbox and star styles when the row selected

Context menu

  • All context menus need to appear on the right side of the target point.
    The context menu is usually placed on the right/bottom side of the mouse click unless it is close to the right age of the screen, then it will appear on the left side. However, in VSCode it seems like always on the right side, but we can do it correctly I think.
  • Remove the triangle (on top) from the context menus
@shcheklein shcheklein added bug Something isn't working priority-p1 Regular product backlog A: experiments Area: experiments table webview and everything related 🔍 review A placeholder issue to review certain part of the product or story labels Aug 23, 2022
@shcheklein shcheklein changed the title EXPs table style corrections Review: table styles Aug 23, 2022
@julieg18 julieg18 assigned julieg18 and unassigned julieg18 Aug 23, 2022
@maxagin
Copy link
Contributor Author

maxagin commented Aug 23, 2022

When the first cell is on hover, the checkbox is not visible because it has the same colour as the cell’s BG colour. See Figma

Screen Shot 2022-08-23 at 4 14 36 PM

Screen Shot 2022-08-23 at 4 28 21 PM

During the column resize, the resize icon and the vertical line (resize indicator) have to stay visible all the time if the user still holds the mouse button.
I am also attaching an example from wandb.ai

Screen.Recording.2022-08-23.at.4.18.17.PM.mov
Screen.Recording.2022-08-23.at.4.22.18.PM.mov

The workspace row height in Figma is a bit larger than other rows. This was done so the row will stand out from the other rows.

Screen Shot 2022-08-23 at 4 26 25 PM

When resizing a column the separator line is a different colour in Figma

Screen Shot 2022-08-23 at 4 30 49 PM

I thought we have agreed that when the cell is on hover we show just left and right borders keeping the same BG colour. This is without any relation to the column hover style.

Screen Shot 2022-08-23 at 4 36 14 PM

I don't think we need the table header closing right line. See below.

Screen Shot 2022-08-23 at 4 37 02 PM

The checkpoint lines should be placed on top of the row border.

Screen Shot 2022-08-23 at 4 40 13 PM

The BG of the view is different in Figma.

Screen Shot 2022-08-23 at 4 42 20 PM

When the header section/s is on hover, hover styles need to be applied (see Figma).

Screen Shot 2022-08-23 at 4 44 27 PM

When star an experiment the weird progress lines show on the sidebar. I think this is something that was already reported.

Screen.Recording.2022-08-23.at.4.48.57.PM.mov

The Figma uses different styles for the row selection. Not sure if we have a special ticket for this.

Screen Shot 2022-08-23 at 4 52 05 PM

.

In relation to the Experiments table: improve styles #2070

@maxagin maxagin self-assigned this Aug 23, 2022
@maxagin
Copy link
Contributor Author

maxagin commented Aug 24, 2022

Hi @julieg18 . I have added to Figma the new styles "Number of hidden elements indicators" when changing the selection styles this also needs to be corrected. Thank you!

Screen Shot 2022-08-24 at 1 30 51 PM

@julieg18
Copy link
Contributor

Created a task list:

  • Add tooltips to checkboxes and stars
  • Keep the resize icon and the vertical line (resize indicator) visible during column resize
  • Increase the workspace row height
  • Update on-hover header separator line background color
  • Take off table header right closing line
  • Stop sidebar sidebar progress lines when starring a experiment
  • Keep checkpoint lines on top of the row border.
  • Update row selection styles
  • Add hover styles to header cells
  • "Number of hidden element indicators"

When the header section/s is on hover, hover styles need to be applied (see Figma).

When you are referring to hover styles, do you mean changing the background color? Wanted to make sure since we took off that particular style on the body cells :)

Also, due to needing to update multiple cells on hover, this may require too much rerendering 🤔 Will do some research before confirming though :)
image

I have added to Figma the new styles "Number of hidden elements indicators" when changing the selection styles this also needs to be corrected. Thank you!

Hmmm, I'm not quite sure what the task is. Could you provide some more context?

@maxagin
Copy link
Contributor Author

maxagin commented Aug 25, 2022

When the header section/s is on hover, hover styles need to be applied (see Figma).

When you are referring to hover styles, do you mean changing the background color? Wanted to make sure since we took off that particular style on the body cells :)

Yes. You are right about the fact that we have removed the particular style on the body cells, but for the header, it is different, already because the header cells have the lines. Most importantly we would like to have the same hover style for the header cells as we are using for the row on hover.

I have added to Figma the new styles "Number of hidden elements indicators" when changing the selection styles this also needs to be corrected. Thank you!

Hmmm, I'm not quite sure what the task is. Could you provide some more context?

I am adding it to the description like this: A number of hidden element indicators (circle with the number) styles for rested and selected row states. Let me know if you have any questions, please.

@julieg18 I have added a few more things to the TODO list and published them in the description (on top).

Question: I do not understand what this is about: Stop sidebar progress lines when starring an experiment?

@maxagin
Copy link
Contributor Author

maxagin commented Aug 25, 2022

Preparing the table for "Text selection in the table's header and body cells" modifications.

Currently, to show exp in plots you can click on the circle/EXP name. Let's remove the EXP name from being a toggle. In other words, the toggle will be only the circle.

Screen Shot 2022-08-24 at 10 28 27 PM

The description was updated. The new task was included.

@maxagin
Copy link
Contributor Author

maxagin commented Aug 25, 2022

After the horizontal scroll is activated and the shadow appears, the right border of the hovered first cell does not need to be shown.

Screen Shot 2022-08-25 at 1 04 14 AM

The description was updated. The new task was included.

@julieg18
Copy link
Contributor

Question: I do not understand what this is about: Stop sidebar progress lines when starring an experiment?

I was referring to weird sidebar progress bars that you already mentioned (and said were probably already reported):

image

@maxagin
Copy link
Contributor Author

maxagin commented Aug 25, 2022

I was referring to weird sidebar progress bars that you already mentioned (and said were probably already reported):

Yeah @julieg18. Thank you !
The description has been updated.

@julieg18
Copy link
Contributor

Reviewed the comments made in #2289! Looks like there are currently two unresolved ones:

Use the full height of the cell (also I have a feeling the blue line has opacity. Right?):
image

Should be doable and updated the task list!

Very minor: consistent vertical lines
image

Don't think this is currently doable without creating a performance heavy solution :(

@julieg18
Copy link
Contributor

Preparing the table for "Text selection in the table's header and body cells" modifications.
Currently, to show exp in plots you can click on the circle/EXP name. Let's remove the EXP name from being a toggle. In other words, the toggle will be only the circle.

@maxagin, is the goal just to make the text selectable? If so, maybe we should consider making the text selectable but also still clickable. If we make the toggle only the circle, it would be very small and more difficult to click on.

@maxagin
Copy link
Contributor Author

maxagin commented Sep 1, 2022

Preparing the table for "Text selection in the table's header and body cells" modifications.
Currently, to show exp in plots you can click on the circle/EXP name. Let's remove the EXP name from being a toggle. In other words, the toggle will be only the circle.

@maxagin, is the goal just to make the text selectable? If so, maybe we should consider making the text selectable but also still clickable. If we make the toggle only the circle, it would be very small and more difficult to click on.

@julieg18, the active area of any clickable icon needs to be 20x20 (we also use 16x16). For example, the checkbox and star active area are 20x20px. The toggle is the icon (circle). IMO if the trigger is cell or name it is confusing.

Screen Shot 2022-08-31 at 9 07 41 PM

@maxagin
Copy link
Contributor Author

maxagin commented Sep 1, 2022

Very minor: consistent vertical lines
image

@julieg18 Don't think this is currently doable without creating a performance heavy solution :(

I have noticed that some lines are fine some not. Why it's happening?

Screen Shot 2022-08-31 at 9 16 18 PM

@julieg18
Copy link
Contributor

julieg18 commented Sep 1, 2022

I have noticed that some lines are fine some not. Why it's happening?

Due the differences between how the borders are added on the head vs the body. The border on the head cells are adding by applying border-right to each cell. Body cells, on the order hand, get their borders on the right and left sides, causing the slight inconsistency between the placement of the borders...

Actually, now that I type that, I think I have an idea that will fix the lines without javascript... Let me test it out!

The solution appears to be working! Updated the list and should get a pr open in a bit!

@maxagin
Copy link
Contributor Author

maxagin commented Sep 1, 2022

After the vertical scroll is activated and the shadow appears, the stuck section bottom border needs to be removed. Same as we do in similar cases.

Screen Shot 2022-09-01 at 5 44 00 PM

The description was updated. The new task was included.

@maxagin
Copy link
Contributor Author

maxagin commented Sep 2, 2022

First row under the workspace ideas:

  1. There is no point to have a checkbox, star and chevron
  2. If the first is correct: we can move everything a little to the left
  3. Have it sticky
    WDYT @shcheklein ?

@shcheklein
Copy link
Member

@maxagin it should be a separate discussion? let's finally close this one?

@maxagin
Copy link
Contributor Author

maxagin commented Sep 2, 2022

Shabbos Nocturne

  • Chevron on hover - cursor pointer
  • There is a little spacing between the elements in the design, for example, checkbox and star, it is important to show the user where the active area ends and where the new starts
  • The sort arrow in the header cell does need to have BG on hover but needs to have a cursor (pointer)
  • Click on the Sort arrow - the context menu appears - if you hover on the arrow again the tooltip will be shown on top of the context menu. Do not show a tooltip when the menu is opened
  • All context menus need to appear on the right side of the target point
  • Remove the triangle (on top) from the context menus
  • Hints like Select, Add Star and similar. The text is too big. Try to make it the same as when you hover on the Experiments tab
  • Hints need to be closer to the cursor
  • Hints do not need to have a triangle at the top

p.s.
Love how height the task list becomes. Would like to see at what point Babylon will fall. @shcheklein, have you ever had a chance to be in a position of a God?

Bob.Marley.-.Babylon.System.mp4

The description was updated. The new task was included. If Babylon will fall today, we will create a new ticket!

@maxagin
Copy link
Contributor Author

maxagin commented Sep 2, 2022

Add tooltip: Add/Remove to plots (may be different wording) when the EXP's circle has hovered.

The description was updated. The new task was included.

@maxagin
Copy link
Contributor Author

maxagin commented Sep 2, 2022

The D&D cursor appears when the header block has hovered. Blocks like "Create" have no active space on top. Let's make sure that all the space of the block can be used to D&D, including train.py from the attached example.

Screen Shot 2022-09-02 at 4 50 55 PM

The description was updated. The new task was included.

@mattseddon
Copy link
Member

mattseddon commented Sep 6, 2022

When star an experiment the weird progress lines show on the sidebar. (Bug?)

Was fixed by #2042

Edit: The behaviour was reverted because we now provide an option to filter to starred.

@maxagin
Copy link
Contributor Author

maxagin commented Sep 6, 2022

@julieg18 please post the links to PRs . I am having a hard time finding it. Thanks

@maxagin
Copy link
Contributor Author

maxagin commented Sep 7, 2022

The description was updated. Please take a look.

@mattseddon
Copy link
Member

Can we move the header updates into #2256?

@maxagin maxagin mentioned this issue Sep 7, 2022
11 tasks
@julieg18
Copy link
Contributor

julieg18 commented Sep 7, 2022

@julieg18 please post the links to PRs . I am having a hard time finding it. Thanks

Sure!

#2289:

  • add tooltips to row actions (checkbox and stars)
  • take off first cell right border when the cell is sticky
  • increase workspace row height
  • take off right border on the head rows last children
  • update column resizer hover background color

#2316:

  • keep checkpoint dotted lines above border
  • keep head cell and body cell borders lining up correctly
  • turn off workspace row border when sticky
  • update table container background

#2305:

  • show columnResizer bar active styles on hover and when the column is being resized
  • keep the resizer bar the same height of the cell(s)

@maxagin
Copy link
Contributor Author

maxagin commented Sep 7, 2022

  • The table's last row does not need a button border

The description was updated. The new task was included.

@julieg18
Copy link
Contributor

julieg18 commented Sep 8, 2022

Checkbox and star styles when the row selected

I can update the star styles (#2351) but the checkbox is different from the star since a vscode component instead of a custom component.

We could figure out a way to update it with some JavaScript but I don't think it's necessary. VSCode keeps the checkbox on default styles when you select rows in the tree as well:

image

@maxagin
Copy link
Contributor Author

maxagin commented Sep 12, 2022

I am not sure it makes sense to have all stars (checked and unchecked) look the same. I think this negates the purpose of this functionality.

Screen Shot 2022-09-12 at 4 59 14 PM

@shcheklein
Copy link
Member

I am not sure it makes sense to have all stars (checked and unchecked) look the same. I think this negates the purpose of this functionality.

Yep, it can be a bit dangerous. Actions itself is fine, but I would expect to have undo for this, or an option to get back.

Otherwise someone from time to time will be clicking by an accident thus removing carefully created state.

@julieg18
Copy link
Contributor

I am not sure it makes sense to have all stars (checked and unchecked) look the same. I think this negates the purpose of this functionality.
Yep, it can be a bit dangerous. Actions itself is fine, but I would expect to have undo for this, or an option to get back.
Otherwise someone from time to time will be clicking by an accident thus removing carefully created state.

Are we referring to the color of the star icon? We can adjust the opacity to make an unchecked star partially opaque again, hopefully to the point where it's still viewable across most themes 🤔

@shcheklein
Copy link
Member

Okay, I might have missed the issue that @maxagin was referring to. I was thinking about mass actions like select experiments - star/unstar, toggle plots. They could be destructive in nature and they are one mistake away (one wrong click) cc @mattseddon .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: experiments Area: experiments table webview and everything related bug Something isn't working priority-p1 Regular product backlog 🔍 review A placeholder issue to review certain part of the product or story
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants