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

Update cell hover styles #2198

Merged
merged 4 commits into from
Aug 19, 2022
Merged

Update cell hover styles #2198

merged 4 commits into from
Aug 19, 2022

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented Aug 16, 2022

2/2 main <- #2197 <- this

  • add cell horizontal borders instead of updating cell background color, fixing invisible (or near invisible) checkboxes in some themes.

Example of main cell hover style:

image

Example of PR cell hover style:

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

Part of #2133 followup

@julieg18 julieg18 added the bug Something isn't working label Aug 16, 2022
@julieg18 julieg18 requested a review from maxagin August 16, 2022 19:19
@julieg18 julieg18 self-assigned this Aug 16, 2022
@julieg18 julieg18 force-pushed the update-cell-hover-style branch from e564bb3 to 5dd030c Compare August 16, 2022 19:59
@julieg18 julieg18 marked this pull request as ready for review August 16, 2022 20:11
@julieg18 julieg18 requested a review from shcheklein August 16, 2022 20:23
@shcheklein
Copy link
Member

I don't like tbh, how text is moving a few pixels as you hover. Is it a bug or a feature?

@shcheklein
Copy link
Member

Color also doesn't change - is it expected?

@julieg18
Copy link
Contributor Author

julieg18 commented Aug 16, 2022

I don't like tbh, how text is moving a few pixels as you hover. Is it a bug or a feature?

Whoops, wrong video! Text shouldn't be moving now.

Color also doesn't change - is it expected?

Yes, we originally wanted to add a background color to the cells, but I'm having trouble finding a background color that works well across themes 🤔

Another option I considered was having the cell hover background be the table color, with or without borders:

image

image

@julieg18 julieg18 mentioned this pull request Aug 16, 2022
Base automatically changed from improve-table-styles to main August 17, 2022 15:18
@julieg18 julieg18 force-pushed the update-cell-hover-style branch from 5dd030c to 1cc0f1c Compare August 18, 2022 23:50
@@ -166,12 +162,6 @@ $workspace-row-edge-margin: $edge-padding - $cell-padding;
border-bottom-color: $row-hover-background-color;
}

.runningExperiment:not(.rowSelected) .experimentCell:hover & {
Copy link
Member

Choose a reason for hiding this comment

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

High contrast light theme

image

Dive bar:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured out a way to fix the background, though the spinners border are still a slight different color. You can't really notice unless you squint, so should be good to merge :)

image

@codeclimate
Copy link

codeclimate bot commented Aug 19, 2022

Code Climate has analyzed commit 9066afc 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.6% (0.0% change).

View more on Code Climate.

@julieg18 julieg18 merged commit dde6e88 into main Aug 19, 2022
@julieg18 julieg18 deleted the update-cell-hover-style branch August 19, 2022 17:49
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