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

Repository table CSS tweaks #106

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

KonradStanski
Copy link
Contributor

@KonradStanski KonradStanski commented Dec 3, 2024

This adds a few CSS tweaks to the repository search table.

We add a table-fixed property and a column width for the name column. This prevents the table layout from jumping around when changing pages which have different max repo name lengths.

Added a pagination page counter, and moved the previous and next buttons to the top right.

Get the table to be responsive and grow to fill the available screen space

The changes are mostly dynamic, so don't really show in before and after pictures, so try this out and change the page size and shape. Should work for various zoom levels.

Before:
Screenshot 2024-12-02 at 9 52 11 PM

Screenshot 2024-12-02 at 9 52 21 PM

After:
Screenshot 2024-12-02 at 9 52 49 PM

@KonradStanski
Copy link
Contributor Author

Thinking about this, I should have maybe split this into 2 diff pr's. The first part is making the table jump around less when changing pages and moving the buttons up top.
The second change of making the table dynamically change length based on window size is... more complicated, and perhaps not the perfect solution. If it's not what we want, I can go and remove it.

@brendan-kellam
Copy link
Contributor

Thinking about this, I should have maybe split this into 2 diff pr's. The first part is making the table jump around less when changing pages and moving the buttons up top. The second change of making the table dynamically change length based on window size is... more complicated, and perhaps not the perfect solution. If it's not what we want, I can go and remove it.

Up to you - I'm also cool with this going in as one PR.

There might be a simpler solution where we can make the table itself scrollable and then assign it a fixed height. This might be more flexible for other scenarios in the future when we aren't dealing with a table that is expected to fill the vertical space of the screen.. but no strong preference one way or the other.

@KonradStanski
Copy link
Contributor Author

should be good to go, just maybe squash merge since I messed up the git history pretty bad.

@@ -33,7 +34,7 @@ export const columns: ColumnDef<RepositoryColumnInfo>[] = [
return (
<div className="flex flex-row items-center gap-2">
<span
className={!isRemoteRepo ? "cursor-pointer text-blue-500 hover:underline": ""}
className={"whitespace-normal break-all " + (!isRemoteRepo ? "cursor-pointer text-blue-500 hover:underline" : "")}
Copy link
Contributor

Choose a reason for hiding this comment

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

micro nit: usually when adding conditional CSS, we use clsx

@@ -47,7 +47,7 @@ const DropdownMenuSubContent = React.forwardRef<
<DropdownMenuPrimitive.SubContent
ref={ref}
className={cn(
"z-50 min-w-[8rem] overflow-hidden rounded-md border bg-popover p-1 text-popover-foreground shadow-lg data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2",
"z-50 min-w-[6rem] overflow-hidden rounded-md border bg-popover p-1 text-popover-foreground shadow-lg data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of modifying the component directly here, could we pass the min-w-[6rem] in the className property of the DropdownMenu?


return (
<div>
<div className="flex items-center py-4">
<div className="pb-12">
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could we add a optional className props to DataTableProps, and then have RepositoryTable pass down this class?

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.

2 participants