Skip to content

Conversation

@julieg18
Copy link
Contributor

@julieg18 julieg18 commented Aug 2, 2022

  • add cell hover style
  • add row hover styles
  • add a bottom border to every row
  • remove even row bg color
  • keep header bg color the same as body bg
  • use the same line style for header item separators
  • keep row arrow the same color as left icons
  • update workspace row, making it sticky and using default row styles
Screen.Recording.2022-08-15.at.9.54.16.AM.mov

Part of #2070, Fixes #2032

* add cell hover style
* add a bottom line every row
* remove even row bg style
* keep header bg same as body bg
* use the same line style for header item separators
* keep row arrow same color as left icons
* update workspace row, making it sticky and using default row styles
@julieg18 julieg18 added the product PR that affects product label Aug 2, 2022
@julieg18 julieg18 requested review from maxagin and shcheklein August 2, 2022 20:09
@julieg18 julieg18 self-assigned this Aug 2, 2022
@shcheklein
Copy link
Contributor

@julieg18 please, put a small demo / video in the description.

@mattseddon
Copy link
Contributor

Looks like there are some changes on the plots side:

image

Were these intended changes?

@julieg18
Copy link
Contributor Author

julieg18 commented Aug 2, 2022

Were these intended changes?

Accidental! Should be fixed now!

Copy link
Contributor Author

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some comments based on the discussion in our demo meeting!

* keep all colors in variables.scss
* remove unneeded cx()
* keep hooks at top
@julieg18
Copy link
Contributor Author

julieg18 commented Aug 3, 2022

Had a call with Maxim and went over some desired changes to the design. Going to convert this into a draft for now!

@julieg18 julieg18 marked this pull request as draft August 3, 2022 16:04
@julieg18 julieg18 closed this Aug 10, 2022
@julieg18 julieg18 reopened this Aug 10, 2022
@julieg18 julieg18 marked this pull request as ready for review August 10, 2022 00:57
@julieg18 julieg18 marked this pull request as draft August 10, 2022 00:57
@julieg18 julieg18 marked this pull request as ready for review August 10, 2022 15:09
@julieg18
Copy link
Contributor Author

With column styles taken off and styles updated (based off the latest Figma), this pr is ready for another review!

@sroy3
Copy link
Contributor

sroy3 commented Aug 15, 2022

This is default Light High Contrast theme. I don't think we should spend too much time on one specific theme, but I believe high contrast themes should stay high contrast for accessibility
Screen Shot 2022-08-15 at 9 17 22 AM

@julieg18
Copy link
Contributor Author

julieg18 commented Aug 15, 2022

I don't think we should spend too much time on one specific theme, but I believe high contrast themes should stay high contrast for accessibility

True! Updated the background color to a variable that is viewable on all themes! Though It does end up being the same (or similar) color as the table on the light themes:

image

image

image

$watermark-color: var(--vscode-descriptionForeground);

$border-color: var(--vscode-dropdown-border);
$border-color: var(--checkbox-border);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Any ideas why some of these aren't prefixed with vscode?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized this actually 😄, I'll update this variable with the vscode prefixed version in the follow-up.

.runningExperiment.oddRow > .experimentCell & {
border-left: 1.5px solid $row-bg-alt-color;
border-bottom: 1.5px solid $row-bg-alt-color;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I] Couple of cases to follow up on:

  1. Middle of an unselected experiment's circle whilst the row is highlighted

Screen Shot 2022-08-16 at 9 33 44 am

  1. Middle of a running experiment's spinner whilst highlighted

Screen Shot 2022-08-16 at 9 32 42 am

Copy link
Contributor

@mattseddon mattseddon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to merge this now, would be good to get the follow-up items addressed before we ship this.

@qlty-cloud-legacy
Copy link

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

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

This pull request will bring the total coverage in the repository to 96.7% (0.0% change).

View more on Code Climate.

@julieg18 julieg18 merged commit ba5b8b5 into main Aug 16, 2022
@julieg18 julieg18 deleted the update-table-styles branch August 16, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

product PR that affects product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include the Workspace row into the fixed section along with the table header

5 participants