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

Add shadow to table head #1835

Merged
merged 12 commits into from
Jun 10, 2022
Merged

Add shadow to table head #1835

merged 12 commits into from
Jun 10, 2022

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented Jun 4, 2022

This PR aims to add styles to the sticky head and first column of the experiments table to make it easier to delineate them, especially when scrolling.

For the header, it's a shadow that uses the same color as other items. For the experiment column, it's a right-side border that uses the same styles as the border used for pinned images in the comparison table. Neither of these disappear at the default position, but that can be added if it's an effect we want.

shadow-and-border-demo.mp4

@mattseddon
Copy link
Member

[Q] Can we match the approach/style that we've taken/used for the sticky column in the comparison table?

@rogermparent rogermparent self-assigned this Jun 7, 2022
@rogermparent
Copy link
Contributor Author

[Q] Can we match the approach/style that we've taken/used for the sticky column in the comparison table?

I'm not sure the exact aspects you're referring to, but I'll take a look at it and adopt what I can. I doubt there'll be many issues doing so 👍

@mattseddon
Copy link
Member

This one:

image

Comment on lines 38 to 47
<div key={column.id}>
<TableHeader
orderedColumns={orderedColumns}
column={column}
columns={columns}
sorts={sorts}
onDragOver={onDragUpdate}
onDragStart={onDragStart}
onDrop={onDragEnd}
/>
</div>
<TableHeader
key={column.id}
orderedColumns={orderedColumns}
column={column}
columns={columns}
sorts={sorts}
onDragOver={onDragUpdate}
onDragStart={onDragStart}
onDrop={onDragEnd}
/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the wrapper div makes styling easier, and it didn't do much other than band-aid a sizing issue that was solved in an alternate way with height: auto.

Comment on lines 111 to 114
),
style: {
position: undefined
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

react-table assigns position:relative by default, but that behavior can be suppressed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set it to static or initial? Maybe it's possible to move it with the CSS as well?

Copy link
Contributor Author

@rogermparent rogermparent Jun 9, 2022

Choose a reason for hiding this comment

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

It must be undefined here because these styles get set as inline styles for every cell and can't be overridden with CSS. We can set position to relative, static, or anything else with CSS if we want an explicit default.

.tr {
position: relative;
& > *:first-child {
background-color: $row-bg-color;
position: sticky;
left: 0;
z-index: 3;
border-right: 2px solid var(--editor-foreground-transparency-4);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same color and width as the one in the comparison table, though that one is implemented with a pseudo-element. Visually, though, they're the same.

@rogermparent rogermparent marked this pull request as ready for review June 9, 2022 03:27
@rogermparent rogermparent added the product PR that affects product label Jun 9, 2022
Comment on lines 111 to 114
),
style: {
position: undefined
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set it to static or initial? Maybe it's possible to move it with the CSS as well?

@mattseddon mattseddon requested review from maxagin and shcheklein June 9, 2022 04:52
Copy link
Member

@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.

Will conflict with #1760. Please can we get that in first and fix the conflicts in this PR.

@shcheklein
Copy link
Member

(not a blocker to merge this) I think that we still need to have a shadow in this case though, it's hard to tell when you see a "scrolled" state if there is something or not underneath the first column. An example of some light shadows:

Screen Shot 2022-06-08 at 11 03 28 PM

Screen Shot 2022-06-08 at 11 03 15 PM

@mattseddon
Copy link
Member

mattseddon commented Jun 9, 2022

Please do follow up and drop the shadow unless scroll is active... or I'll have to change theme 😢:

image

☝🏻 is the change to the experiment column header intentional? I can see from the description it is. Maybe I'm biased by the fact it has looked the same way for so long but I would rather have a consistent color for the header.

@rogermparent
Copy link
Contributor Author

Changing the experiment column's header cell actually wasn't intentional, it just happened because I was using the same selector for both head and body rows and needed a background color in order to not have things showing up behind when overlapped. I just added separate head and body styles that maintain the same background colors we had before.

As far as making the shadow disappear, I'll start on that after the table-wide sort/filter indicators.

onDrop={onDragEnd}
/>
</div>
<TableHeader
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Jun 10, 2022

Code Climate has analyzed commit 8efd216 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

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.

@rogermparent rogermparent merged commit d22127e into main Jun 10, 2022
@rogermparent rogermparent deleted the sticky-shadows branch June 10, 2022 20:27
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.

4 participants