Skip to content

Conversation

@julieg18
Copy link
Contributor

@julieg18 julieg18 commented Aug 16, 2022

1/2 main <- this <- #2198

  • add vscode prefix to --checkbox-border variable
  • keep cell spinners and unselected circles the same bg color as cells
  • keep the sticky column from losing opacity on row hover

Example of cell spinners and unselected circles

Screen.Recording.2022-08-16.at.11.02.58.AM.mov

Example of now fully opaque experiment column

Screen.Recording.2022-08-16.at.12.55.31.PM.mov

Followup to #2133

* add vscode prefix to `--checkbox-border`
* keep cell spinners and unselected circles the same bg color as cells
@julieg18 julieg18 added the bug Something isn't working label Aug 16, 2022
@julieg18 julieg18 self-assigned this Aug 16, 2022
Copy link
Contributor

@sroy3 sroy3 left a comment

Choose a reason for hiding this comment

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

Should we change the border to be the same as the row background as well?

@julieg18
Copy link
Contributor Author

Should we change the border to be the same as the row background as well?

Apologies, I'm not sure what border you're referring to?

@sroy3
Copy link
Contributor

sroy3 commented Aug 16, 2022

Should we change the border to be the same as the row background as well?

Apologies, I'm not sure what border you're referring to?

Screen Shot 2022-08-16 at 11 05 14 AM

The black border on the spinner where the cursor is.

@julieg18
Copy link
Contributor Author

The black border on the spinner where the cursor is.

Oh, I see what you're saying! I'll need to rethink this solution a bit more. Now that I'm looking at things a second time, I just noticed that our checkboxes are not visible on the default theme on hover as well:

image

We may need to rethink our hover background colors again 😅

* top sticky exp column from losing opacity on hover
* properly fix colors on spinners and bullets
@julieg18
Copy link
Contributor Author

julieg18 commented Aug 16, 2022

Properly fixed the background for spinners/bullets and made sure that the exp column always retains full opacity no matter the background color(some row background colors are partially transparent, causing this issue in some themes):

image

@julieg18 julieg18 requested a review from sroy3 August 16, 2022 17:16
@shcheklein
Copy link
Contributor

I think things like this should be fixed, folks. Unless I'm missing something and it's an actual edge case (e.g. there are places in VS Code with this theme where it breaks the interface).

Screen Shot 2022-08-16 at 10 37 24 AM

I'm not sure why is it happening btw. @julieg18 could you give a bit more details. What variable do we use for background that causes this? In what case (hover, active, etc?)

@julieg18
Copy link
Contributor Author

I think things like this should be fixed, folks. Unless I'm missing something and it's an actual edge case (e.g. there are places in VS Code with this theme where it breaks the interface).
I'm not sure why is it happening btw. @julieg18 could you give a bit more details. What variable do we use for background that causes this? In what case (hover, active, etc?)

The PR does indeed fix this bug if I wasn't clear before (I'll go ahead and add an extra video, showcasing this)! Here's how things look now:

image

As for why it's happening, the variable we are using for row hover is called --vscode-list-hoverBackground which depending on the theme, is sometimes using a color that is partially transparent. Due to the partially transparent color being used as a background on row hover, the sticky column becomes partially transparent and you can see the columns underneath, causing the bug.

I fixed the issue by keeping the background of the column the same color as the table (a color that is never transparent) and used an absolute :before element to change the background color, ensuring that the column always stays opaque.

@shcheklein
Copy link
Contributor

Good, thanks @julieg18 !

@mattseddon
Copy link
Contributor

Still a slight issue with the row being selected:

image

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

$border-color: var(--checkbox-border);
$border-color: var(--vscode-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.

There is at least one more of these in this file. --button-primary-background

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird, that variable actually doesn't have a vscode prefixed version 🤔

@qlty-cloud-legacy
Copy link

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

The test coverage on the diff in this pull request is 100.0% (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
Copy link
Contributor Author

Still a slight issue with the row being selected:

Fixed!

@julieg18 julieg18 merged commit b16cd85 into main Aug 17, 2022
@julieg18 julieg18 deleted the improve-table-styles branch August 17, 2022 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants