-
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
chore(charts): readable tooltip value #331
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request focuses on improving the readability and functionality of chart tooltips in the generic-charts components, as well as refactoring the action menu in the data table component. The changes primarily affect the bar and area charts, introducing more detailed and customizable tooltip content. Additionally, there are modifications to the action menu structure to improve toast notifications handling. 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 - here's some feedback:
Overall Comments:
- The
<Toaster />
component has been removed from the DataTable. Please ensure it's been added higher up in the component tree to maintain toast functionality.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 3 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
}: ActionButtonProps<TData, TValue>) { | ||
const { toast, dismiss } = useToast() | ||
toast, | ||
dismiss, |
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 (complexity): Consider reverting to using useToast directly within the function.
The recent changes to the ActionItem
function have increased its complexity by adding toast
and dismiss
as parameters. This makes the function more tightly coupled with the useToast
hook and increases the risk of errors if these props are not provided correctly. To simplify, consider reverting to the original approach where useToast
is used directly within the function. This keeps the component self-contained and reduces redundancy, improving maintainability and readability.
<ChartTooltip | ||
cursor | ||
content={ | ||
<ChartTooltipContent |
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 (complexity): Consider breaking down the JSX into smaller components.
The new code introduces increased complexity due to additional nesting, inline styles, and verbose logic. Consider breaking down the JSX and logic into smaller, reusable components to improve readability and maintainability. For example, extracting the tooltip content into a separate CustomTooltipContent
component can help reduce complexity and enhance separation of concerns. This approach will make the code easier to read and maintain, while also improving reusability.
index, | ||
payload: Array<Payload<ValueType, NameType>> | ||
) => { | ||
return ( |
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 (complexity): Consider refactoring the JSX logic into a helper function.
The new code introduces increased complexity due to additional nesting, verbose syntax, and some redundant logic. To improve readability and maintainability, consider refactoring by extracting the repeated JSX logic into a helper function, as shown in the renderContent
function. This reduces redundancy and simplifies the code structure. Also, ensure consistent styling throughout the code to make it easier to follow. These changes should help in making the code more straightforward and easier to maintain.
Summary by Sourcery
Enhance the readability of tooltips in bar and area charts by formatting values and adding visual indicators. Refactor the ActionItem and ActionMenu components to improve toast notification handling. Remove the Toaster component from the DataTable component.
Enhancements:
Chores: