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

chore: align tailwind tables with ui design #4371

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

connoratrug
Copy link
Contributor

@connoratrug connoratrug commented Oct 17, 2024

What are the main changes you did:

  • Align tabels page with design
  • remove content block container
  • add bg color to table
  • add border radius to table
  • set margen as per design
  • remove debug loading indicator

how to test:

  • explain here what to do to test this (or point to unit tests)

todo:

  • updated docs in case of new feature
  • added/updated tests
  • added/updated testplan to include a test for this fix, including ref to bug using # notation

Copy link
Member

@mswertz mswertz left a comment

Choose a reason for hiding this comment

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

what to do with rounded borders limiting text?
image

@connoratrug
Copy link
Contributor Author

connoratrug commented Oct 17, 2024

what to do with rounded borders limiting text?

i know , tried some stuff , but did not work or side effect , hoping @davidruvolo51 may have a solution
tr in displaytable does not allow for last:pb-2 or something , may we could a another wrapper ( but that has some other effect)

maybe some kind a css mask can work , but wanted to focus on the general page layout and components first

Copy link
Member

@mswertz mswertz left a comment

Choose a reason for hiding this comment

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

ok, we solve the issue with rounded corners later

Copy link
Contributor

@davidruvolo51 davidruvolo51 left a comment

Choose a reason for hiding this comment

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

Looks great!

I added a suggestion the rounded table border and spacing issue in the last row.

@click="handleSortRequest(column.id)"

<div class="overflow-auto rounded-b-50px">
<div class="overflow-x-auto overscroll-x-contain bg-table rounded-t-3px">
Copy link
Contributor

Choose a reason for hiding this comment

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

To have enough spacing at the bottom, it's possible to add pb-* here. In this design, I noticed that the text hugs the left side of the table. We could consider adding a little bit of space in the first cell in each row.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a yes , pb-* 'solves' it , not 100% on the best value here, the adding space in the first cell may not really work as the tables scrolls horizontally ( but maybe col 1 needs to be stick or the left edge needs a fade styling when scolling)

</th>
</tr>
</thead>
<tbody class="mb-3">
Copy link
Contributor

Choose a reason for hiding this comment

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

To remove the border from the last row, you can structure the class like this -

<tbody class="mb-3 [&_tr:last-child_td]:border-none">

I found that the text feels compressed. We could also add a little bit of space on the bottom:

<tbody class="mb-3 [&_tr:last-child_td]:border-none [&_tr:last-child_td]:mb-5">

@connoratrug
Copy link
Contributor Author

@davidruvolo51 i made small tweaks , feel free to add / edit , for now i think this will do , once we have more themes and table features implemented we can improve on style details

Copy link

sonarcloud bot commented Oct 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.

3 participants