feat: better columns display for related charts #239
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @duyet - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 9 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -18,13 +18,16 @@ export async function RelatedCharts({ | |||
| className, | |||
| }: RelatedChartsProps) { | |||
| // Related charts | |||
| const charts: [ComponentType<ChartProps>, ChartProps][] = [] | |||
| type ChartName = string | |||
There was a problem hiding this comment.
suggestion: Consider using a more descriptive type name.
The type alias ChartName could be more descriptive to improve readability. For example, ChartComponentName might be clearer.
| type ChartName = string | |
| type ChartComponentName = string |
| </ServerComponentLazy> | ||
| ))} | ||
| {charts.map(([_name, Chart, props], i) => { | ||
| let className = '' |
There was a problem hiding this comment.
suggestion: Consider using a more descriptive variable name.
The variable className could be more descriptive, such as chartClassName, to better convey its purpose.
| let className = '' | |
| let chartClassName = '' |
| // | chart1 | | ||
| // | chart2 | chart3 | | ||
| // ----------------------- | ||
| if (charts[i + 1] && charts[i + 1][0] === 'break') { |
There was a problem hiding this comment.
suggestion: Consider extracting logic to a helper function.
The logic for determining the className could be extracted to a helper function to improve readability and maintainability.
| if (charts[i + 1] && charts[i + 1][0] === 'break') { | |
| const shouldSpanTwoColumns = (charts, index) => charts[index + 1] && charts[index + 1][0] === 'break'; | |
| if (shouldSpanTwoColumns(charts, i)) { | |
| className = 'col-span-2'; | |
| } |
| // | chart1 | chart2 | | ||
| // | chart3 | | ||
| // ----------------------- | ||
| if (charts.length > 2 && i === charts.length - 1 && col === 2) { |
There was a problem hiding this comment.
question: Clarify the TODO comment.
The TODO comment mentions implementing for maxChartsPerRow > 2. Consider adding more details or creating a task to ensure this is addressed.
| @@ -21,6 +21,9 @@ interface ReloadButtonProps { | |||
| className?: string | |||
| } | |||
|
|
|||
| const SECOND = 1000 | |||
| const MINUTE = 60 * SECOND | |||
There was a problem hiding this comment.
suggestion: Consider using a more descriptive constant name.
The constant MINUTE could be renamed to MILLISECONDS_IN_MINUTE for better clarity.
| const MINUTE = 60 * SECOND | |
| const MILLISECONDS_IN_MINUTE = 60 * SECOND |
| relatedCharts?: | ||
| | string[] | ||
| | [string, ChartProps][] | ||
| | (string | [string, ChartProps])[] |
There was a problem hiding this comment.
suggestion: Clarify the new type definition.
The new type definition for relatedCharts is more complex. Consider adding a comment to explain the different possible structures for better readability.
| | (string | [string, ChartProps])[] | |
| // relatedCharts can be: | |
| // 1. An array of strings | |
| // 2. An array of tuples, where each tuple contains a string and ChartProps | |
| // 3. An array containing both strings and tuples | |
| | (string | [string, ChartProps])[] |
| props = {} | ||
| props: ChartProps = {} | ||
|
|
||
| if (!chart) continue |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!chart) continue | |
| if (!chart) { |
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).
components/related-charts.tsx
Outdated
| @@ -33,24 +36,53 @@ export async function RelatedCharts({ | |||
| props = chart[1] | |||
| } | |||
|
|
|||
| if (!component) continue | |||
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!component) continue | |
| if (!component) { |
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).
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: Duyet Le <5009534+duyet@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: Duyet Le <5009534+duyet@users.noreply.github.com>
No description provided.