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.

Sign up for GitHub

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(ui): alternative table extras show current table name instead of database #259

Merged
merged 1 commit into from
May 28, 2024
Merged

chore(ui): alternative table extras show current table name instead of database #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/database/[database]/[table]/extras/alternative-tables.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export async function AlternativeTables({
className="group flex flex-row gap-2 text-muted-foreground"
>
<CardStackMinusIcon className="size-3" />
{database}
{table}
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Potential variable name confusion

The change from {database} to {table} might be intentional, but it could lead to confusion if the context is not clear. Ensure that this change aligns with the intended logic and that the variable names are consistent and descriptive.

<ChevronDownIcon
className="size-3 transition duration-300 group-data-[state=open]:rotate-180"
aria-hidden="true"
Expand Down
15 changes: 15 additions & 0 deletions app/play/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { ErrorAlert } from '@/components/error-alert'
import { SingleLineSkeleton, TableSkeleton } from '@/components/skeleton'
Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (performance): Consider lazy loading components

For performance optimization, consider lazy loading the components if they are not immediately needed. This can help reduce the initial load time of the page.

Suggested change
import { ErrorAlert } from '@/components/error-alert'
import { SingleLineSkeleton, TableSkeleton } from '@/components/skeleton'
import dynamic from 'next/dynamic'
const ErrorAlert = dynamic(() => import('@/components/error-alert'))
const SingleLineSkeleton = dynamic(() => import('@/components/skeleton').then(mod => mod.SingleLineSkeleton))
const TableSkeleton = dynamic(() => import('@/components/skeleton').then(mod => mod.TableSkeleton))


export default function Play() {
return (
<>
<TableSkeleton />
<TableSkeleton cols={5} />
<ErrorAlert message="Error message" query="SELECT 1" />
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Hardcoded error message and query

Consider making the error message and query dynamic or configurable. Hardcoding these values can limit the reusability and flexibility of the ErrorAlert component.

Suggested change
<ErrorAlert message="Error message" query="SELECT 1" />
const errorMessage = "Error message";
const errorQuery = "SELECT 1";
<ErrorAlert message={errorMessage} query={errorQuery} />

<SingleLineSkeleton />
<SingleLineSkeleton className="w-[300px]" />
<SingleLineSkeleton className="w-[200px] space-x-0 pt-0" />
</>
)
}
Loading