Skip to content

Conversation

@palbizu
Copy link
Contributor

@palbizu palbizu commented Apr 15, 2021

Description

The background color and borders were changed in hover over on a selected table.

Testing

Visual Testing

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

@palbizu palbizu requested a review from a team as a code owner April 15, 2021 18:02
@github-actions
Copy link

Unit Test Results

    4 files  ±0  248 suites  ±0   13m 39s ⏱️ - 2m 13s
889 tests ±0  889 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
895 runs  ±0  895 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 9f1d0c2. ± Comparison against base commit f0295d8.

border-top: 1px solid $blue-2;

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

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

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

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?

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

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

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

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

@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #774 (9f1d0c2) into main (f0295d8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #774   +/-   ##
=======================================
  Coverage   85.38%   85.38%           
=======================================
  Files         789      789           
  Lines       16145    16145           
  Branches     2060     2060           
=======================================
  Hits        13785    13785           
  Misses       2329     2329           
  Partials       31       31           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0295d8...9f1d0c2. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants