Design graph component (CO2 Captured and Canopy Cover) #1984
Conversation
1. add project launch text 2. add dynamic values to tooltip
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
can you make the marked portion lighter instead of bold ? |
mohitb35
force-pushed
the
feature/redesign-explore-btn
branch
from
c7d4a2b to
9d0b734
Compare
mohitb35
force-pushed
the
feature/redesign-explore-btn
branch
3 times, most recently
from
f54228e to
9ea7417
Compare
mariahosfeld
dismissed
their stale review
Dismissed due to remarks concerning changes in design that were approved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (15)
src/theme/_fonts.scss (1)
14-14: LGTM! Consider reordering for consistency.The addition of
$fontXXSmallNewis appropriate and consistent with the existing naming convention. It expands the available font size options, which aligns well with the PR objectives of enhancing design capabilities.For improved readability and consistency, consider reordering the variables to group similar sizes together. You might want to place
$fontXXSmallNewnear other "small" font size variables, such as$fontXXSmallor$fontXSmall.src/temp/stories/Graph.stories.tsx (5)
4-8: LGTM: Metadata is correctly defined.The metadata for the Graph component is properly typed and exported. Consider adding more properties to the metadata object for better documentation and control in Storybook.
You could enhance the metadata by adding properties like
title,tags, orargTypes. For example:const meta: Meta<typeof Graph> = { component: Graph, title: 'Components/Graph', tags: ['autodocs'], argTypes: { type: { control: 'select', options: ['carbonCapture', 'canopyCover'], }, }, };
11-11: Consider making the years array more flexible.The years array is efficiently defined for use in both stories. However, consider making it more dynamic to accommodate future updates easily.
You could generate the years array dynamically:
const currentYear = new Date().getFullYear(); const years = Array.from({length: 5}, (_, i) => currentYear - 4 + i);This approach would automatically update the years range as time passes.
13-20: LGTM: CarbonCapture story is well-structured.The CarbonCapture story is correctly defined with appropriate data for carbon capture visualization.
Consider adding more properties to the args object to showcase different scenarios or edge cases. For example:
export const CarbonCapture: Story = { args: { years: years, byProjectResult: [23.4, 23.27, 23.78, 23.7, 23.78], regionalAverage: [22.54, 22.65, 21.8, 21.85, 22.03], type: 'carbonCapture', title: 'Carbon Capture Over Time', subtitle: 'Comparison of Project Results vs Regional Average', }, };This would allow testing of the Graph component with different titles and subtitles.
22-29: LGTM: CanopyCover story is well-structured.The CanopyCover story is correctly defined with appropriate data for canopy cover visualization.
Similar to the CarbonCapture story, consider adding more properties to the args object:
export const CanopyCover: Story = { args: { years: years, byProjectResult: [23, 24, 25, 26, 27], regionalAverage: [19, 20, 21, 22, 23], type: 'canopyCover', title: 'Canopy Cover Progress', subtitle: 'Project Results vs Regional Average', }, };This would provide more comprehensive testing scenarios for the Graph component.
1-29: Great job on the Storybook stories implementation!The file is well-structured and follows Storybook best practices. It provides two distinct stories for different graph types, which is excellent for testing and showcasing the Graph component's versatility.
To further enhance the stories, consider adding a third story that demonstrates how the Graph component handles edge cases or extreme data scenarios. For example:
export const EdgeCase: Story = { args: { years: [2020, 2021, 2022], byProjectResult: [0, 100, 50], regionalAverage: [50, 50, 50], type: 'carbonCapture', title: 'Edge Case Scenario', subtitle: 'Handling Extreme Data Points', }, };This would help ensure the Graph component can gracefully handle a wider range of data inputs.
src/temp/CarbonCapture/Graph.module.scss (2)
1-36: Replace hardcoded colors with theme variablesThe use of nested styles is good for readability. However, there are hardcoded color values that should be replaced with theme variables for better maintainability and consistency across the application.
Please replace the following hardcoded colors with appropriate theme variables:
- color: #333; //to be replaced + color: var(--color-text-primary); - background-color: rgba(var(--primary-color-new), 0.1); + background-color: var(--color-background-tooltip); - color: $secondaryColorNew; + color: var(--color-secondary); - color: $dark; + color: var(--color-text-dark);Ensure that these variables are defined in your theme file.
38-63: Improve color usage and text transformationThe layout using flexbox is well-structured. However, there are a few improvements that can be made:
- Replace hardcoded colors with theme variables for consistency:
- color: #2f3336; //to be replaced + color: var(--color-text-primary); - color: #bdbdbd; //to be replaced + color: var(--color-text-secondary);
- Consider using
text-transform: capitalize;instead oflowercasefor.graphSubheading:- text-transform: lowercase; + text-transform: capitalize;This will ensure that the first letter of each word is capitalized, which is generally more appropriate for headings.
src/theme/_colors.scss (1)
40-40: LGTM! Consider grouping similar color variables.The addition of
$secondaryColorNewis correct and follows the existing naming convention. It expands the color palette and maintains the use of CSS custom properties for flexible theming.For improved readability and maintenance, consider grouping this new variable with other "New" color variables (e.g.,
$primaryColorNew). This would enhance the organization of the file.src/theme/themeProperties.ts (1)
38-38: LGTM: New secondary color property added.The addition of
secondaryColorNewwith the value '#377E36' is a good enhancement to the theme properties. This dark green color complements the primary color well.If this is part of a larger color scheme update, consider adding a comment or updating the documentation to explain the purpose and usage of this new secondary color.
src/theme/theme.ts (3)
19-19: LGTM! Consider adding dark theme support for consistency.The addition of
secondaryColorNewenhances the theme's flexibility. The implementation is consistent with other color variables.For complete consistency, consider adding support for this new color in the dark theme as well:
.theme-dark { // ... existing properties ... + --secondary-color-new: ${dark.secondaryColorNew}; }Also applies to: 82-82
64-64: LGTM! Consider adding a comment for clarity.The addition of
--font-xx-extra-small-newincreases the granularity of font size options. The implementation is consistent with other font size variables.For improved clarity, consider adding a comment explaining the difference between this new font size and the existing
--font-xx-extra-small:+ // New, slightly adjusted xx-extra-small font size for specific use cases --font-xx-extra-small-new: ${fontSizes.fontXXSmallNew};
Line range hint
1-145: Consider refactoring theme structure for improved maintainability.The current structure mixes light and dark theme properties, with some properties defined only for the light theme. This could lead to inconsistencies in dark mode.
Consider refactoring the theme structure to ensure consistency between light and dark modes:
- Define a base set of properties common to both themes.
- Create separate objects for light and dark theme-specific properties.
- Use a function to merge the base properties with theme-specific ones.
This approach would make it easier to maintain and extend the theme in the future, ensuring all properties are consistently defined for both modes.
src/temp/CarbonCapture/Graph.tsx (2)
132-139: Clarify marker colors for consistencyIn the
markersconfiguration, the use of template literals with unnecessary interpolation may reduce code clarity.Simplify the color assignments:
markers: { size: 0, - colors: [`${light.light}`, 'transparent'], - strokeColors: [`${primaryDarkColorX}`, 'transparent'], + colors: [light.light, 'transparent'], + strokeColors: [primaryDarkColorX, 'transparent'], strokeOpacity: [1, 1], strokeWidth: 2.2, hover: { size: 6, }, },
70-79: Initialize state with a function to prevent unnecessary re-rendersWhen using
useState, initializing state with a function can prevent unnecessary recalculations during re-renders.Modify the state initialization:
- const [xaxisOptions, setXaxisOptions] = useState< - (number | (string | number)[])[] - >([]); + const [xaxisOptions, setXaxisOptions] = useState(() => { + return years.map((year, index) => { + if (index === 1) { + return [year, ` ${t('projectLaunch')}`]; + } else { + return year; + } + }); + });And remove the
useEffecthook if it's no longer needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
.github/workflows/chromatic.ymlis excluded by!**/*.ymlpublic/static/locales/en/projectDetails.jsonis excluded by!**/*.json
📒 Files selected for processing (7)
- src/temp/CarbonCapture/Graph.module.scss (1 hunks)
- src/temp/CarbonCapture/Graph.tsx (1 hunks)
- src/temp/stories/Graph.stories.tsx (1 hunks)
- src/theme/_colors.scss (1 hunks)
- src/theme/_fonts.scss (1 hunks)
- src/theme/theme.ts (3 hunks)
- src/theme/themeProperties.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
src/temp/stories/Graph.stories.tsx (2)
1-2: LGTM: Imports are correct and appropriate.The necessary Storybook types and the Graph component are properly imported.
9-9: LGTM: Story type is correctly defined.The Story type is properly defined using StoryObj, which is the recommended approach for typing Storybook stories.
src/temp/CarbonCapture/Graph.module.scss (2)
69-75: LGTM: New info icon stylesThe styles for
.newInfoIconare well-defined. The use of flexbox for alignment and thecursor: pointerproperty appropriately indicate that the icon is interactive.
1-75: Overall: Well-structured SCSS with minor improvement suggestionsThe SCSS for the graph component and its tooltip is well-organized and follows good practices. The use of nested styles, flexbox for layout, and consistent font size variables contributes to maintainable and readable code.
To further improve the code:
- Replace hardcoded color values with theme variables throughout the file.
- Consider capitalizing the graph subheading instead of using lowercase.
These minor adjustments will enhance maintainability and visual consistency across the application.
src/theme/themeProperties.ts (1)
7-7: LGTM: New font size property added.The new
fontXXSmallNewproperty with a value of '9px' is a good addition to the existing font size hierarchy. It maintains consistency with the naming convention of other font size properties.
| @@ -29,11 +30,12 @@ const themeProperties = { | |||
| topProjectBackgroundColor: '#F3BB44', | |||
| nonDonatableProjectBackgroundColor: '#828282', | |||
| deforestrationRangeBackgroundNew: '235, 87, 87', | |||
| primaryColorNew: '#219653', | |||
| primaryColorNew: '33, 150, 83', | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Remaining instances of '#219653' found.
The old hex color #219653 is still used in the following locations:
- src/theme/themeProperties.ts
- public/assets/images/ProfilePageIcons/index.tsx
- src/features/user/Profile/styles/MyForestMap.module.scss
- src/features/user/Profile/styles/MyForest.module.scss
- src/features/projects/styles/ProjectSnippet.module.scss
- src/features/projects/components/ProjectSnippet.tsx
- src/features/projects/components/PopupProject.tsx
Please update these instances to use the new RGB format.
🔗 Analysis chain
LGTM: Primary color updated to RGB format.
The change from hex (#219653) to RGB (33, 150, 83) for primaryColorNew is good. It provides more flexibility for color manipulation and transparency.
Please verify that this change doesn't break any existing color usage in the application. Run the following script to check for any remaining hex color usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining hex color usage of #219653
# Test: Search for the old hex color. Expect: No results.
rg -i '#219653'
Length of output: 1172
| chart: { | ||
| type: 'area', | ||
| width: 300, | ||
| toolbar: { | ||
| show: false, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Remove hardcoded chart width for responsiveness
The chart's width is hardcoded to 300, which may not render well on different screen sizes. Allowing the chart to adapt to its container improves responsiveness.
Remove the hardcoded width:
chart: {
type: 'area',
- width: 300,
toolbar: {
show: false,
},
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| chart: { | |
| type: 'area', | |
| width: 300, | |
| toolbar: { | |
| show: false, | |
| }, | |
| }, | |
| chart: { | |
| type: 'area', | |
| toolbar: { | |
| show: false, | |
| }, | |
| }, |
| useEffect(() => { | ||
| const newOptions = years.map((year, index) => { | ||
| if (index === 1) { | ||
| return [2020, ` ${t('projectLaunch')}`]; | ||
| } else { | ||
| return year; | ||
| } | ||
| }); | ||
| setXaxisOptions(newOptions); | ||
| }, []); |
There was a problem hiding this comment.
Avoid hardcoding values and update dependency array
In the useEffect hook, the use of a hardcoded year (2020) when setting x-axis options can lead to incorrect labeling if the actual years differ. Additionally, the dependency array is empty, which may cause issues if years or t (translations) change.
Apply this diff to use dynamic year values and update the dependency array:
useEffect(() => {
const newOptions = years.map((year, index) => {
if (index === 1) {
- return [2020, ` ${t('projectLaunch')}`];
+ return [year, ` ${t('projectLaunch')}`];
} else {
return year;
}
});
setXaxisOptions(newOptions);
-}, []);
+}, [years, t]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| const newOptions = years.map((year, index) => { | |
| if (index === 1) { | |
| return [2020, ` ${t('projectLaunch')}`]; | |
| } else { | |
| return year; | |
| } | |
| }); | |
| setXaxisOptions(newOptions); | |
| }, []); | |
| useEffect(() => { | |
| const newOptions = years.map((year, index) => { | |
| if (index === 1) { | |
| return [year, ` ${t('projectLaunch')}`]; | |
| } else { | |
| return year; | |
| } | |
| }); | |
| setXaxisOptions(newOptions); | |
| }, [years, t]); |
| formatter: function (index: number) { | ||
| if (index === 2) { | ||
| return xaxisOptions[1]; | ||
| } else if (index == xaxisOptions.length) { | ||
| return xaxisOptions[index - 1]; | ||
| } else { | ||
| return ''; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Correct x-axis label formatter function
The x-axis label formatter function uses index, but ApexCharts provides value to the formatter. This may result in labels not displaying correctly.
Modify the formatter function to use value:
labels: {
- formatter: function (index: number) {
- if (index === 2) {
- return xaxisOptions[1];
- } else if (index == xaxisOptions.length) {
- return xaxisOptions[index - 1];
+ formatter: function (value: number, timestamp?: number, opts?: any) {
+ const dataPointIndex = opts?.dataPoints?.findIndex(
+ (point: any) => point.x === value
+ );
+ if (dataPointIndex === 1) {
+ return xaxisOptions[1][1];
+ } else if (dataPointIndex === xaxisOptions.length - 1) {
+ return xaxisOptions[dataPointIndex];
} else {
return '';
}
},This ensures labels are displayed correctly based on the x-axis values.
Committable suggestion was skipped due to low confidence.
| custom: function ({ dataPointIndex, w }: CustomTooltipProps) { | ||
| const getToolTip = () => { | ||
| const headingTranslation = t('co2Removed', { | ||
| value: w.globals.series[0][dataPointIndex], | ||
| }); | ||
| const subHeadingTranslation = t('biomass', { | ||
| value: w.globals.series[1][dataPointIndex], | ||
| }); | ||
| const yoyTranslation = t('yoy', { | ||
| value: 4, | ||
| }); | ||
| const canopyCoverPercentage = 43; | ||
| const dataPoint = xaxisOptions[dataPointIndex]; | ||
| const year = Array.isArray(dataPoint) ? dataPoint[0] : dataPoint; | ||
| const date = year.toString(); | ||
|
|
||
| return ( | ||
| <Tooltip | ||
| headerTitle={headingTranslation} | ||
| subTitle={subHeadingTranslation} | ||
| yoyValue={yoyTranslation} | ||
| date={date} | ||
| type={type} | ||
| canopyCoverPercentage={canopyCoverPercentage} | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
| return ReactDOMServer.renderToString(getToolTip()); | ||
| }, |
There was a problem hiding this comment.
Use dynamic data in tooltip content
Within the custom tooltip function, the values for yoyTranslation and canopyCoverPercentage are hardcoded (value: 4 and canopyCoverPercentage = 43). This can result in inaccurate tooltip information. Consider calculating these values based on the actual data to ensure accuracy.
Update the code to use dynamic values:
const getToolTip = () => {
const headingTranslation = t('co2Removed', {
value: w.globals.series[0][dataPointIndex],
});
const subHeadingTranslation = t('biomass', {
value: w.globals.series[1][dataPointIndex],
});
- const yoyTranslation = t('yoy', {
- value: 4,
- });
- const canopyCoverPercentage = 43;
+ const currentValue = w.globals.series[0][dataPointIndex];
+ const previousValue = w.globals.series[0][dataPointIndex - 1] || currentValue;
+ const yoyValue = ((currentValue - previousValue) / previousValue) * 100;
+ const yoyTranslation = t('yoy', {
+ value: yoyValue.toFixed(2),
+ });
+ const canopyCoverPercentage = currentValue;
const dataPoint = xaxisOptions[dataPointIndex];
const year = Array.isArray(dataPoint) ? dataPoint[0] : dataPoint;
return (
<Tooltip
headerTitle={headingTranslation}
subTitle={subHeadingTranslation}
yoyValue={yoyTranslation}
date={year.toString()}
type={type}
canopyCoverPercentage={canopyCoverPercentage}
/>
);
};Ensure that previousValue handles cases where there is no previous data point.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| custom: function ({ dataPointIndex, w }: CustomTooltipProps) { | |
| const getToolTip = () => { | |
| const headingTranslation = t('co2Removed', { | |
| value: w.globals.series[0][dataPointIndex], | |
| }); | |
| const subHeadingTranslation = t('biomass', { | |
| value: w.globals.series[1][dataPointIndex], | |
| }); | |
| const yoyTranslation = t('yoy', { | |
| value: 4, | |
| }); | |
| const canopyCoverPercentage = 43; | |
| const dataPoint = xaxisOptions[dataPointIndex]; | |
| const year = Array.isArray(dataPoint) ? dataPoint[0] : dataPoint; | |
| const date = year.toString(); | |
| return ( | |
| <Tooltip | |
| headerTitle={headingTranslation} | |
| subTitle={subHeadingTranslation} | |
| yoyValue={yoyTranslation} | |
| date={date} | |
| type={type} | |
| canopyCoverPercentage={canopyCoverPercentage} | |
| /> | |
| ); | |
| }; | |
| return ReactDOMServer.renderToString(getToolTip()); | |
| }, | |
| custom: function ({ dataPointIndex, w }: CustomTooltipProps) { | |
| const getToolTip = () => { | |
| const headingTranslation = t('co2Removed', { | |
| value: w.globals.series[0][dataPointIndex], | |
| }); | |
| const subHeadingTranslation = t('biomass', { | |
| value: w.globals.series[1][dataPointIndex], | |
| }); | |
| const currentValue = w.globals.series[0][dataPointIndex]; | |
| const previousValue = w.globals.series[0][dataPointIndex - 1] || currentValue; | |
| const yoyValue = ((currentValue - previousValue) / previousValue) * 100; | |
| const yoyTranslation = t('yoy', { | |
| value: yoyValue.toFixed(2), | |
| }); | |
| const canopyCoverPercentage = currentValue; | |
| const dataPoint = xaxisOptions[dataPointIndex]; | |
| const year = Array.isArray(dataPoint) ? dataPoint[0] : dataPoint; | |
| return ( | |
| <Tooltip | |
| headerTitle={headingTranslation} | |
| subTitle={subHeadingTranslation} | |
| yoyValue={yoyTranslation} | |
| date={year.toString()} | |
| type={type} | |
| canopyCoverPercentage={canopyCoverPercentage} | |
| /> | |
| ); | |
| }; | |
| return ReactDOMServer.renderToString(getToolTip()); | |
| }, |
Component:
Carbon CaptureCanopy coverDesign changes:
Left: