feat: 🎸 Add sparkline rendering function #139
Conversation
This commit adds the `renderDistribution2` function, which is used to render a kernel density estimation (KDE) sparkline chart within a given HTML element.
Summary of ChangesHello @fnlearner, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new renderDistribution2 function to create kernel density estimation (KDE) sparkline charts, along with several new utility functions for data processing, path creation, and scales. The overall implementation is a great addition.
My review includes several suggestions to improve the code quality, maintainability, and correctness. I've identified a critical bug in renderDistribution2 that could cause the chart to render twice. I've also pointed out opportunities to improve code clarity by renaming interfaces, refactoring complex logic, adding documentation, and using more descriptive error messages. Additionally, there are suggestions to enhance type safety in some of the new utility functions.
src/charts/sparkline/index.tsx
Outdated
| // 使用新的 createSvg 函数来创建和选择 SVG 元素 | ||
| const svgD3 = createSvg(container, width, height); | ||
|
|
||
| const pathData = createCurvePath(xScale, finalYScale, density); | ||
|
|
||
| svgD3 | ||
| .append('path') | ||
| .attr('class', 'mypath') | ||
| .datum(density) | ||
| .attr('fill', 'none') | ||
| .attr('stroke', 'steelblue') | ||
| .attr('stroke-width', 2) | ||
| .attr('stroke-linejoin', 'round') | ||
| .attr('d', pathData); |
There was a problem hiding this comment.
This block of code is executed unconditionally. However, if wordElement is provided and sparkLinePosition is 'up' or 'down', the sparkline is already rendered inside the if block at lines 84-120. This causes the sparkline to be rendered twice. This block should be wrapped in an else statement corresponding to the if on line 84 to prevent this.
src/charts/sparkline/index.tsx
Outdated
| import { createCurvePath, createSvg, extent, max, mean, scaleLinear } from '../utils'; | ||
| import { ticks } from '../utils/scales'; | ||
|
|
||
| export interface Distribution1Config { |
There was a problem hiding this comment.
The interface is named Distribution1Config, but it's used for the renderDistribution2 function. This is confusing and could lead to errors. Please rename it to Distribution2Config for clarity and consistency, and update its usage on line 16.
| export interface Distribution1Config { | |
| export interface Distribution2Config { |
src/charts/sparkline/index.tsx
Outdated
| /** | ||
| * | ||
| * @param container | ||
| * @param config | ||
| */ |
There was a problem hiding this comment.
The JSDoc for renderDistribution2 is empty. Please add a description of what the function does, its parameters (container, config), and any important details about its behavior, such as how it renders the KDE sparkline. Good documentation is crucial for maintainability.
src/charts/sparkline/index.tsx
Outdated
| * @param container | ||
| * @param config | ||
| */ | ||
| export const renderDistribution2 = (container: Element, config: Distribution1Config) => { |
There was a problem hiding this comment.
Following the suggestion to rename the Distribution1Config interface, please update the type here to Distribution2Config.
| export const renderDistribution2 = (container: Element, config: Distribution1Config) => { | |
| export const renderDistribution2 = (container: Element, config: Distribution2Config) => { |
src/charts/sparkline/index.tsx
Outdated
| height = 20; | ||
| } | ||
|
|
||
| // 清理旧的SVG |
There was a problem hiding this comment.
This comment is in Chinese, while the rest of the codebase is in English. For consistency and to ensure all developers can understand the comments, please translate it to English.
// Clear old SVG
src/charts/sparkline/index.tsx
Outdated
|
|
||
| const valueExtent = extent(values); | ||
| if (valueExtent[0] === undefined) { | ||
| throw new Error('Error'); |
There was a problem hiding this comment.
The error message "Error" is not descriptive. When an error is thrown, it should provide context about what went wrong. Please provide a more informative error message.
throw new Error('Input data is empty or invalid, cannot calculate value extent.');
src/charts/sparkline/index.tsx
Outdated
|
|
||
| const xScale = scaleLinear(valueExtent, [padding, width - padding]); | ||
|
|
||
| const kde = kernelDensityEstimator(kernelEpanechnikov(7), ticks(valueExtent, 40)); |
There was a problem hiding this comment.
The numbers 7 and 40 are used here without explanation, making the code harder to understand. Please define them as named constants with comments explaining their purpose (e.g., KDE_BANDWIDTH, TICK_COUNT).
const KDE_BANDWIDTH = 7; // Controls the smoothness of the KDE plot.
const TICK_COUNT = 40; // Number of points to sample for the density estimation.
const kde = kernelDensityEstimator(kernelEpanechnikov(KDE_BANDWIDTH), ticks(valueExtent, TICK_COUNT));
src/charts/utils/data.ts
Outdated
| if (values.length === 0) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
This check for values.length === 0 is redundant because the check !array || array.length === 0 at the beginning of the function already handles empty arrays. You can safely remove this block.
src/charts/utils/selection.ts
Outdated
| nodes(): Element[] { | ||
| return [...this.elements]; | ||
| } | ||
| datum(data): Selection { |
There was a problem hiding this comment.
The data parameter in the datum method has an implicit any type. To improve type safety, you should provide an explicit type. Using a generic D would be the most flexible and type-safe approach.
| datum(data): Selection { | |
| datum<D>(data: D): Selection { |
There was a problem hiding this comment.
Code Review
This pull request introduces a new renderDistribution2 function for rendering kernel density estimation sparkline charts. The changes include adding the new function, updating imports and exports, and adding a mock data file. The review focuses on potential improvements in error handling and code clarity.
src/charts/utils/selection.ts
Outdated
| // Store the data on the element, a common D3 practice | ||
| // You can also use a private field on the Selection instance. | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (el as any).__data__ = data; |
There was a problem hiding this comment.
Storing data directly on DOM elements using __data__ is generally discouraged as it pollutes the DOM and can lead to conflicts with other libraries or frameworks. Consider using a WeakMap to associate data with DOM elements instead. This provides a cleaner and more encapsulated way to manage data.
Here's how you can implement this using a WeakMap:
const dataMap = new WeakMap<Element, any>();
this.elements.forEach((el) => {
dataMap.set(el, data);
}); const dataMap = new WeakMap<Element, any>();
this.elements.forEach((el) => {
dataMap.set(el, data);
});
return this;
src/charts/sparkline/index.tsx
Outdated
| if (valueExtent[0] === undefined) { | ||
| throw new Error('Error'); |
There was a problem hiding this comment.
This error handling could be improved. Throwing a generic Error object doesn't provide much context. Consider providing a more specific error message to aid in debugging. Also, it might be better to return null or an empty svg element instead of throwing an error, to prevent the entire chart from failing to render if the data is invalid.
For example, the error message could be Data extent is undefined, cannot render chart.
src/charts/utils/paths.ts
Outdated
| }; | ||
| }; | ||
|
|
||
| // A simplified implementation of d3.curveBasis |
There was a problem hiding this comment.
This comment is redundant. It's restating the obvious. Remove it to improve code clarity.
src/charts/sparkline/index.tsx
Outdated
| while (container.firstChild) { | ||
| container.removeChild(container.firstChild); | ||
| } |
There was a problem hiding this comment.
Consider using container.innerHTML = ''; instead of the while loop for clearing the SVG. This is generally faster and more concise.
container.innerHTML = '';
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
- Coverage 91.48% 85.57% -5.91%
==========================================
Files 67 68 +1
Lines 1139 1227 +88
Branches 235 235
==========================================
+ Hits 1042 1050 +8
- Misses 97 177 +80
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
3 participants
This commit adds the
renderDistribution2function, which is used to render a kernel density estimation (KDE) sparkline chart within a given HTML element.