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

Fix table overflow and pagination rounded border #3031

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

nelsonkopliku
Copy link
Member

Description

the overflow hidden added to the table component in this PR #3010 prevented the items per page selection to be properly shown.

I moved the necessary classname to the pagination component.

image

@nelsonkopliku nelsonkopliku self-assigned this Sep 27, 2024
@nelsonkopliku nelsonkopliku added bug Something isn't working env Create an ephimeral environment for the pr branch labels Sep 27, 2024
@jagabomb
Copy link
Contributor

jagabomb commented Sep 27, 2024

@nelsonkopliku I like how this fixes the usability of the dropdown component. I think we would still need to apply to rounded classes to some additional sections so that we have rounded corners on the edges of the table component.

Here are a couple of examples were it is missing the rounded effect:
Populated/Sorted
Screenshot 2024-09-27 at 12 10 57

With Header
Screenshot 2024-09-27 at 12 11 16

Another suggestion would be to change the position of the dropdown since the CSS in the previous PR creates a mask across all elements within the table. Therefore changing the position of the dropdown to the top of the selector would be quick-fix. 😅

Here is a quick reference:
Screenshot 2024-09-27 at 12 19 32

@nelsonkopliku
Copy link
Member Author

hey @jagabomb thanks for the suggestion, that's pretty smart. However it is not that straightforward changing the position of the options listbox, our current version (1.7) of headless ui does not support that well, while version 2 does, but we still don't have version 2 available #2881

If you have some pointers on how to do what you suggest with what we have currently, please share 😄
I'll take a look at how having also the two scenarios mentioned rounded.

@jagabomb
Copy link
Contributor

@nelsonkopliku Cool, I can imagine headless ui 2.0 is not ready to merge.

I was thinking of a simpler solution, so if we take the Select component and give it a position property with an empty value which doesn't add a class by default but then if a user sets it to top (e.g: position-top ). Then that property could be passed to the Listbox.options component to conditionally add a bottom-11 class. This way we could then position the dropdown only when it sits in the bottom section of the table. Also we can amend this more easily when we add headless ui 2.0 .

@nelsonkopliku nelsonkopliku force-pushed the fix-table-overflow branch 2 times, most recently from bff4644 to f703ec8 Compare September 30, 2024 08:40
@nelsonkopliku
Copy link
Member Author

Awesome @jagabomb! Thanks a lot for the help, I could have lost myself in tailwind to find the correct class 🙈

Copy link
Contributor

@jagabomb jagabomb left a comment

Choose a reason for hiding this comment

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

Awesome! 🚀

@nelsonkopliku nelsonkopliku merged commit 33aab6e into main Sep 30, 2024
30 checks passed
@nelsonkopliku nelsonkopliku deleted the fix-table-overflow branch September 30, 2024 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working env Create an ephimeral environment for the pr branch
Development

Successfully merging this pull request may close these issues.

2 participants