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

Improve table styles #2289

Merged
merged 4 commits into from
Aug 30, 2022
Merged

Improve table styles #2289

merged 4 commits into from
Aug 30, 2022

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented Aug 29, 2022

  • add tooltips to row actions (checkbox and stars)
  • take off first cell right border when the cell is sticky
  • increase workspace row height
  • take off right border on the head rows last children
  • update column resizer hover background color

Demo

Screen.Recording.2022-08-30.at.10.30.04.AM.mov

Part of #2241

* add tooltips to row actions
* take off first cell right border when cell is sticky
* increase workspace row height
* take off right border on last header children
* update column resizer hover background color
@julieg18 julieg18 self-assigned this Aug 29, 2022
@julieg18 julieg18 added the product PR that affects product label Aug 29, 2022
@julieg18 julieg18 marked this pull request as ready for review August 29, 2022 23:08
@julieg18 julieg18 requested review from shcheklein and maxagin August 29, 2022 23:08
@mattseddon
Copy link
Member

mattseddon commented Aug 29, 2022

@julieg18 what happens at ~10 seconds of the video?

From watching the whole video I can assume that selecting the row removes the stickiness of the first column. This looks broken to me.

@julieg18
Copy link
Contributor Author

From watching the whole video I can assume that selecting the row removes the stickiness of the first column. This looks broken to me.

Yes, selecting the row turns off the stickiness. It's always been like that though (picture below was taken on the latest extension release):

image

Agreed though, it does look buggy... I can look into keeping selected rows sticky if we'd like!

@maxagin
Copy link
Contributor

maxagin commented Aug 30, 2022

Agreed though, it does look buggy... I can look into keeping selected rows sticky if we'd like!

Please share the updated version, when you have it. Thank you @julieg18

@maxagin
Copy link
Contributor

maxagin commented Aug 30, 2022

Good work @julieg18
Very minor: consistent vertical lines

Screen Shot 2022-08-29 at 9 26 03 PM

Use the full height of the cell (also I have a feeling the blue line has opacity. Right?):

Screen Shot 2022-08-29 at 9 28 54 PM

@maxagin
Copy link
Contributor

maxagin commented Aug 30, 2022

In relation to the

take off first cell right border when the cell is sticky

Screen Shot 2022-08-29 at 9 36 12 PM

@maxagin
Copy link
Contributor

maxagin commented Aug 30, 2022

Just FYI. The selected first cell does not have a shadow:

Screen Shot 2022-08-29 at 9 39 16 PM

@maxagin
Copy link
Contributor

maxagin commented Aug 30, 2022

Q: is the blue border when on hover 2px?

Screen Shot 2022-08-29 at 9 42 10 PM

@julieg18
Copy link
Contributor Author

julieg18 commented Aug 30, 2022

Use the full height of the cell (also I have a feeling the blue line has opacity. Right?):

Correct! I'll take the opacity off. Not sure if I can make the lines the full height of the cell though, will need to look into it a bit more 🤔

Q: is the blue border when on hover 2px?

Yes, it is 2px!

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.

Since what was previously mentioned by Matt is out of scope. This LGTM! I would still follow up on it though.

@julieg18 julieg18 reopened this Aug 30, 2022
@maxagin maxagin mentioned this pull request Aug 30, 2022
27 tasks
@codeclimate
Copy link

codeclimate bot commented Aug 30, 2022

Code Climate has analyzed commit fc48e40 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.9% (0.0% change).

View more on Code Climate.

@julieg18
Copy link
Contributor Author

julieg18 commented Aug 30, 2022

Ok, merged #2294 which fixed the none sticky selected rows issue!

Also, per @maxagin comments, added opacity to the resizer bar and took off the borders when the header cells are sticky since those were both pretty simple fixes. Video is updated and I think we're good to merge once tests are passing!

@julieg18 julieg18 merged commit 82599a9 into main Aug 30, 2022
@julieg18 julieg18 deleted the table-styles-review branch August 30, 2022 16:40
@maxagin
Copy link
Contributor

maxagin commented Sep 1, 2022

Looks like "The selected first cell does not have a shadow" also was fixed :) #2289 (comment)
Good stuff @julieg18 !

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