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

fixed table header chevron layout shift #2766

Closed
wants to merge 54 commits into from

Conversation

kwongz
Copy link
Contributor

@kwongz kwongz commented Nov 12, 2024

Description

First page is new datatable, second page is old datatable
https://www.loom.com/share/5608209a1dfe479f89c7dbb3475de4f1?sid=37b1a37f-9c53-4c2b-9c68-18f4da575669

Checklist

  • For UI or styling changes, I have added a screenshot or gif showing before & after
  • I have added a changeset
  • I have added to the docs where applicable
  • I have added to the VS Code extension where applicable

Copy link

changeset-bot bot commented Nov 12, 2024

🦋 Changeset detected

Latest commit: dfc035a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@evidence-dev/core-components Patch
my-evidence-project Patch
e2e-prerender Patch
e2e-spa Patch
e2e-themes Patch
@evidence-dev/components Patch
evidence-test-environment Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kwongz kwongz requested a review from hughess November 12, 2024 19:29
@hughess
Copy link
Member

hughess commented Nov 12, 2024

The layout shift looks a lot better, but unfortunately we'll need to make sure the column title and the values in the column are aligned - for example in a numeric column they should both be right aligned, so any gap you're applying to the column title should also be applied to the individual cells in the column.

That might work well if we also reduce the left/right padding in the cells, and let the new gutter use that space

@kwongz
Copy link
Contributor Author

kwongz commented Nov 12, 2024

Agreed i think this will look much better

improved datatable header

@kwongz
Copy link
Contributor Author

kwongz commented Nov 13, 2024

Aligned table cell content with table header, removed the existing padding and used the "gutter" for spacing between columns

image

image

@hughess
Copy link
Member

hughess commented Nov 13, 2024

This looks good in general, but there's a few cases where I think we need to adjust.

Before sending over those, just a quick clarifying question - is wrapTitles applied in this table? https://docs-i8kx0rshh-evidence.vercel.app/components/data-table/#column-groups

@kwongz
Copy link
Contributor Author

kwongz commented Nov 13, 2024

Column Groups example i removed wraptitles from the code example. I see I forgot to remove it from the code block.

It seems not necessary since we have wraptitle example just below

@hughess
Copy link
Member

hughess commented Nov 13, 2024

Here's a few scenarios that look a bit off:

  1. Not enough padding on left side of cells
    CleanShot 2024-11-13 at 16 52 52@2x

  2. Unclear which column the sort icon applies to
    CleanShot 2024-11-13 at 16 51 41@2x
    CleanShot 2024-11-13 at 16 49 42@2x

  3. Too much padding on right side of cells - is more apparent when conditional formatting is used
    CleanShot 2024-11-13 at 16 50 36@2x

I think the solution is some combo of:

  • Increase padding on left side of cells
  • Reduce space between title and left side of chevron icon
  • (possibly) reduce size of chevron icon
  • (possibly) reduce space on right of chevron

@kwongz
Copy link
Contributor Author

kwongz commented Nov 13, 2024

Sounds good ill get these out right now

@kwongz
Copy link
Contributor Author

kwongz commented Nov 14, 2024

Hey @hughess made some of your changes, sorry couldn't finish these last night, wanted to clean up some things before showing it to you

  • cleaned up spacing between cells
  • reduced size of chevron
  • added this idea of a gradient background to highlight the sorted column + indicate asc or desc sort
  • added margin left to chev, I thought it gets too close to header when datatable have many columns

without the current margin left, chev will stick next to header text with many columns
image

Table cell padding-right was 19px, reduced to 12px
table cell spacing
image

other datatables

image

table cell centered text
image

image

@hughess
Copy link
Member

hughess commented Nov 14, 2024

Ah, I think because we don't have lines around cells, the shading implementation ends up looking out of place. I think we'll need to stick with just the icon to avoid highlighting the visual difference between the title cell and the row cells.

What happened to the alignment behaviour on column titles? Looks like when align=center, the column title is staying on the left. Looks like it's the same when align=right, so I assume the alignment just isn't taking effect on the title cell at all atm

@kwongz
Copy link
Contributor Author

kwongz commented Nov 14, 2024

ah i c, to fix the wrapped titles I wrapped them in a flex box structure to get the chevron to behave properly.

ill fix

  • Align center header text
  • remove linear gradient
  • Do we want the chev to always be on the right in the "gutter" or do we want it to be right next to header text?

@kwongz
Copy link
Contributor Author

kwongz commented Nov 14, 2024

  • Fixed the alignment issue
    image

  • more spacing left of chevron then right (compared to right adjacent table header text)
    image


-Something I dislike is alignment center, the chev is staying on the right, when the datatable has a lot of space the chev feels unconnected to that table. We Could allow alignment center chevs to be centered as well

alignment= center: columns country ID chev feels disconnected
image

Solutions: center chev when alignment is center
image

I think this is the case even when text is default aligned: left, if the column has a large width, there s a lot of empty white space in between text and chev. Maybe the chev should follow the text alignment?

Current: align=left
image

_Proposal: chev follows alignment rule
image

@kwongz
Copy link
Contributor Author

kwongz commented Nov 15, 2024

fixes #2711

@hughess
Copy link
Member

hughess commented Nov 18, 2024

Getting close!

  • When the column is right-aligned now, the title has extra padding that's not present in the cells:
    CleanShot 2024-11-18 at 18 48 45@2x

  • The chevron looks a bit far from the text in the title:
    CleanShot 2024-11-18 at 18 50 05@2x

  • When columns are close together, it's too hard to tell which column the chevron belongs too - need reduced padding/margin on left side of chevron
    CleanShot 2024-11-18 at 18 50 55@2x

Removing the extra space from my first bullet might solve the other 2

@kwongz
Copy link
Contributor Author

kwongz commented Nov 21, 2024

conflicts resolved in #2830

@kwongz kwongz closed this Nov 21, 2024
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.

4 participants