Skip to content

fix: hover over a selected table changed color #774

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: hover over a selected table changed color #774

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions projects/components/src/table/table.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@
border-top: 1px solid $blue-2;

&.hovered-row {
background: $blue-2;
Copy link
Contributor

@anandtiwary anandtiwary Apr 15, 2021

Choose a reason for hiding this comment

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

This is for hover on a selected row. Has the ux changed for this? At all other place, we make the color darker on hover. This would make it inconsistent with similar behavior on a card list

Copy link

@michael-traceable michael-traceable Apr 21, 2021

Choose a reason for hiding this comment

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

Yeah. So hover on these rows right now is a little off. We go darker on buttons typically, but with rows and other navigation items the hover I think would be better suited to always be the same.

Alternatively, I could accept that a hover on a selected row doesn't change anything. Instead it stays as a selected state.

Copy link
Contributor

@anandtiwary anandtiwary Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I am fine with whatever you decide on visual aspects. I just want to share that this change require similar modifications to

  • Sequence chart: d3 row hover. Without this waterfall would appear broken.
  • Card lists

@michael-traceable Could you confirm we need to apply styling at these two locations too?

Copy link

@michael-traceable michael-traceable Apr 21, 2021

Choose a reason for hiding this comment

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

Good call! Yes, waterfall should match because it's similar structure (table format)... The card list we can match as well, however, I believe I also have a ticket that the hover on that should just be the shadow and no gray, but it doesn't bother me too much.

Copy link
Contributor

@jake-bassett jake-bassett Apr 23, 2021

Choose a reason for hiding this comment

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

Should we bundle all of these changes at once? I kind of lean towards yes so there are no inconsistencies.

Copy link
Contributor

@anandtiwary anandtiwary May 3, 2021

Choose a reason for hiding this comment

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

I agree. @palbizu Can we bundle all the changes together in the same PR?

Copy link
Contributor Author

@palbizu palbizu May 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@anandtiwary just to double-check. I should apply the same pattern to
1- Tables (I already did it in this PR)
2- Sequence charts d3 row hover
3- Card lists

border-bottom: 1px solid $blue-5;
border-top: 1px solid $blue-5;
background: $gray-1;
border-bottom: 1px solid $gray-2;
border-top: 1px solid $gray-2;
}
}

Expand Down