-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: refactor typing, add ColumnFormat.HoverCard #275
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request refactors the typing system and introduces a new ColumnFormat.HoverCard. The changes include moving the ColumnFormat and ColumnFormatOptions definitions to a new file, '@/lib/types/column-format', and updating all relevant imports. Additionally, a new HoverCardFormat component is added to handle the new column format, and several components are updated to use this new format. The pull request also includes minor refactoring and improvements in various components, such as updating default values and replacing loading messages with skeleton components. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @duyet - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -31,7 +31,7 @@ interface DatabaseCount { | |||
export async function DatabaseBreadcrumb({ database }: Props) { | |||
// Default | |||
let databases: { name: string; count: number }[] = [ | |||
{ name: database, count: 0 }, | |||
{ name: database, count: -1 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Negative count value might be confusing
Using -1
as a placeholder for loading state might be confusing. Consider using a more explicit value or a separate flag to indicate loading state.
@@ -114,3 +114,10 @@ function Internal({ | |||
</Breadcrumb> | |||
) | |||
} | |||
|
|||
function Count({ count }: { count?: number }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (performance): Consider memoizing the Count component
To avoid unnecessary re-renders, consider memoizing the Count
component using React.memo
.
function Count({ count }: { count?: number }) { | |
import React from 'react'; | |
const Count = React.memo(({ count }: { count?: number }) => { | |
if (count == undefined || count == -1) return 'loading ...'; | |
if (count == 0) return '0 table'; | |
}); |
Loading table detail ... | ||
</div> | ||
) | ||
return <TableSkeleton className="mb-4" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider adding a fallback for TableSkeleton
In case TableSkeleton
fails to load or render, consider adding a fallback UI to improve user experience.
return <TableSkeleton className="mb-4" /> | |
return ( | |
<React.Suspense fallback={<div>Loading table detail ...</div>}> | |
<TableSkeleton className="mb-4" /> | |
</React.Suspense> | |
) |
@@ -28,7 +28,7 @@ export interface QueryConfig { | |||
* ``` | |||
*/ | |||
columnFormats?: { | |||
[key: string]: ColumnFormat | [ColumnFormat, ColumnFormatOptions] | |||
[key: string]: ColumnFormat | ColumnFormatWithArgs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider adding a type guard for ColumnFormatWithArgs
To ensure type safety, consider adding a type guard function to differentiate between ColumnFormat
and ColumnFormatWithArgs
.
export function HoverCardFormat({ row, value, options }: HoverCardProps) { | ||
let { content } = options || {} | ||
|
||
if (!content) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Default content assignment
Consider using a more explicit default value for content
to avoid potential issues with falsy values.
if (!content) { | |
if (content === undefined || content === null) { |
|
||
const HoverCardContent = React.forwardRef< | ||
React.ElementRef<typeof HoverCardPrimitive.Content>, | ||
React.ComponentPropsWithoutRef<typeof HoverCardPrimitive.Content> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider using a more descriptive type alias
Using a type alias for React.ComponentPropsWithoutRef<typeof HoverCardPrimitive.Content>
can improve readability and maintainability.
React.ComponentPropsWithoutRef<typeof HoverCardPrimitive.Content> | |
type HoverCardContentProps = React.ComponentPropsWithoutRef<typeof HoverCardPrimitive.Content>; | |
const HoverCardContent = React.forwardRef< | |
React.ElementRef<typeof HoverCardPrimitive.Content>, | |
HoverCardContentProps | |
>(({ className, align = 'center', sideOffset = 4, ...props }, ref) => ( | |
<HoverCardPrimitive.Content |
@@ -114,3 +114,10 @@ function Internal({ | |||
</Breadcrumb> | |||
) | |||
} | |||
|
|||
function Count({ count }: { count?: number }) { | |||
if (count == undefined || count == -1) return 'loading ...' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces
)
if (count == undefined || count == -1) return 'loading ...' | |
if (count == undefined || count == -1) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
|
||
function Count({ count }: { count?: number }) { | ||
if (count == undefined || count == -1) return 'loading ...' | ||
if (count == 0) return '0 table' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces
)
if (count == 0) return '0 table' | |
if (count == 0) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
function Count({ count }: { count?: number }) { | ||
if (count == undefined || count == -1) return 'loading ...' | ||
if (count == 0) return '0 table' | ||
if (count == 1) return '1 table' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces
)
if (count == 1) return '1 table' | |
if (count == 1) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
Summary by Sourcery
This pull request introduces a new 'HoverCard' column format for data tables, refactors the typing and structure of column formats, and enhances the loading and breadcrumb components for better user experience.