Enhancement: Improved chart loading experience and modal mobile view implementation #2227
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve several components, primarily enhancing layout responsiveness and user experience. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
OchiengPaul442
changed the title
|
New next-platform changes available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
platform/src/common/components/Modal/dataDownload/components/Footer.jsx (2)
Line range hint
35-45: Enhance accessibility for the Clear buttonThe Clear button lacks proper accessibility attributes and semantic HTML.
<button type="button" className="ml-2" + aria-label="Clear selected locations" + role="button" onClick={handleClearSelection} > Clear </button>
Line range hint
57-62: Add accessibility attributes for loading stateThe loading spinner should have proper ARIA attributes to improve screen reader support.
<div className="flex items-center gap-2"> - <span className="animate-spin w-4 h-4 border-2 border-t-transparent border-white rounded-full"></span> + <span + className="animate-spin w-4 h-4 border-2 border-t-transparent border-white rounded-full" + role="status" + aria-label="Loading" + ></span> <span>{btnText || 'Downloading...'}</span>{' '} </div>platform/src/common/components/Modal/dataDownload/index.jsx (2)
96-98: Consider adding max-height constraint for overflowThe overflow handling is good, but without a max-height constraint, the content might stretch beyond the viewport on mobile devices.
Consider adding a max-height constraint:
- <div className="flex relative flex-col overflow-y-auto md:overflow-hidden md:flex-row w-full flex-grow"> + <div className="flex relative flex-col overflow-y-auto max-h-[calc(100vh-120px)] md:max-h-none md:overflow-hidden md:flex-row w-full flex-grow">
Line range hint
84-98: Enhance keyboard navigation accessibilityWhile the modal has good accessibility features, consider adding keyboard navigation enhancements:
- Trap focus within the modal when open
- Add
ESCkey handler for closing- Ensure all interactive elements are reachable via keyboard
Consider using the
@headlessui/reactDialog.Panelcomponent which handles these automatically:- <div className="inline-bloc w-[380px] md:w-auto relative align-bottom bg-white rounded-lg text-left overflow-hidden shadow-xl transform transition-all sm:my-8 sm:align-middle lg:min-h-[658px] lg:min-w-[1020px] max-w-[1020px] h-auto "> + <Dialog.Panel className="inline-bloc w-[380px] md:w-auto relative align-bottom bg-white rounded-lg text-left overflow-hidden shadow-xl transform transition-all sm:my-8 sm:align-middle lg:min-h-[658px] lg:min-w-[1020px] max-w-[1020px] h-auto ">platform/src/common/components/Charts/ChartContainer.jsx (2)
11-11: Add PropTypes validation for component propsWhile the hook import and state initialization look good, the component is missing PropTypes validation for its props. This would help catch potential type-related issues early in development.
Add this at the end of the file:
import PropTypes from 'prop-types'; ChartContainer.propTypes = { chartType: PropTypes.string.isRequired, chartTitle: PropTypes.string.isRequired, height: PropTypes.string, width: PropTypes.string, id: PropTypes.string.isRequired, showTitle: PropTypes.bool, data: PropTypes.array, chartLoading: PropTypes.bool, error: PropTypes.any, refetch: PropTypes.func.isRequired };Also applies to: 42-44
51-65: Consider making the skeleton timeout duration configurableWhile the skeleton loader implementation is solid, the hard-coded 8-second timeout might not be suitable for all network conditions. Consider:
- Making this duration configurable via props
- Adding a minimum display time to prevent flickering
- Potentially using the actual chart loading state instead of a timeout
Here's a suggested improvement:
const ChartContainer = ({ // ... other props + skeletonMinDuration = 1000, + skeletonMaxDuration = 8000, }) => { // ... other code useEffect(() => { let timer; if (chartLoading) { setShowSkeleton(true); } else { - timer = setTimeout(() => { + const timeoutDuration = Math.max( + skeletonMinDuration, + Math.min(skeletonMaxDuration, performance.now() - loadStartTime) + ); + timer = setTimeout(() => { setShowSkeleton(false); - }, 8000); + }, timeoutDuration); } return () => { if (timer) clearTimeout(timer); }; }, [chartLoading]);platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (1)
217-223: Consider further responsive design enhancements.While the current responsive changes are good, here are some suggestions to enhance the mobile experience:
- The fixed height (
h-[658px]) on medium screens might cause issues on smaller laptops. Consider using viewport-relative units or max-height constraints:-<div className="w-auto h-auto md:w-[280px] md:h-[658px] overflow-y-auto md:border-r relative space-y-3 px-4 pt-5 pb-14"> +<div className="w-auto h-auto md:w-[280px] md:max-h-[80vh] overflow-y-auto md:border-r relative space-y-3 px-4 pt-5 pb-14">
- The loading skeleton (in
sidebarSitesContent) could benefit from responsive spacing:-<div className="space-y-4"> +<div className="space-y-2 md:space-y-4">platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (2)
150-150: Consider improving mobile height handlingThe current height setup (
h-auto) on mobile might lead to inconsistent layouts. Consider setting a minimum height for better visual stability.-<div className="w-auto h-auto md:w-[280px] md:h-[658px] overflow-y-auto border-r relative space-y-3 px-4 pt-5 pb-14"> +<div className="w-auto min-h-[400px] h-auto md:w-[280px] md:h-[658px] overflow-y-auto border-r relative space-y-3 px-4 pt-5 pb-14">
260-272: Enhance error feedback with more specific messagesWhile the error handling implementation is good, consider providing more specific error messages to help users understand and resolve issues.
-<p className="text-red-500 font-semibold mb-2"> - Something went wrong. Please try again. -</p> +<p className="text-red-500 font-semibold mb-2"> + {error?.message || 'Unable to load chart data. Please try again.'} +</p> +{error?.details && ( + <p className="text-gray-500 text-sm mb-4"> + {error.details} + </p> +)}platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx (1)
Line range hint
160-190: Consider enhancing error handling and validationThe error handling for different file formats is good, but we could make it more robust:
- Add specific error messages for each file format validation
- Implement error boundaries around PDF generation
- Add type validation for the frequency parameter
Here's a suggested enhancement for the error handling:
if (fileExtension === 'csv') { if (typeof response.data !== 'string') { - throw new Error('Invalid CSV data format'); + throw new Error('Expected CSV string data but received ' + typeof response.data); } } else if (fileExtension === 'json') { if (!response.data || !Array.isArray(response.data.data)) { - throw new Error('Invalid JSON data format'); + throw new Error('Expected JSON array but received ' + + (response.data ? typeof response.data.data : 'undefined')); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
platform/src/common/components/Charts/ChartContainer.jsx(3 hunks)platform/src/common/components/Modal/dataDownload/components/Footer.jsx(3 hunks)platform/src/common/components/Modal/dataDownload/index.jsx(2 hunks)platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx(1 hunks)platform/src/common/components/Modal/dataDownload/modules/DataDownload.jsx(3 hunks)platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx(6 hunks)platform/src/core/utils/useFetchAnalyticsData.jsx(0 hunks)platform/src/pages/analytics/_components/OverView.jsx(2 hunks)
💤 Files with no reviewable changes (1)
- platform/src/core/utils/useFetchAnalyticsData.jsx
🔇 Additional comments (10)
platform/src/common/components/Modal/dataDownload/index.jsx (1)
84-84: Verify desktop breakpoint behavior
While the mobile-first approach with 380px width is good, the desktop min-width: 1020px might cause horizontal scrolling on smaller laptops (e.g., 1024px width screens).
Let's check if there are similar breakpoint patterns in other components:
✅ Verification successful
Desktop breakpoint confirmed
The min-width: 1020px is used exclusively in the Modal component, ensuring that other parts of the application remain unaffected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar min-width patterns that might need adjustment
rg -l "min-w-\[10\d{2}px\]" --type jsx
Length of output: 68
Script:
#!/bin/bash
# Search for similar min-width patterns that might need adjustment
rg -l "min-w-\[10\d{2}px\]" -g "*.jsx"
Length of output: 100
platform/src/common/components/Charts/ChartContainer.jsx (2)
46-50: Clean implementation of outside click handling
The useOutsideClick hook implementation effectively manages both the dropdown visibility and state reset. Good job on keeping it concise and following React best practices.
229-232: Well-structured conditional rendering
The error and loading states are handled in the correct order of precedence, with clean and readable conditional rendering. The implementation follows React best practices.
platform/src/common/components/Modal/dataDownload/modules/AddLocations.jsx (2)
217-217: LGTM! Well-implemented responsive container.
The sidebar container now properly adapts to different screen sizes using Tailwind's responsive utilities. The transition from w-auto h-auto on mobile to fixed dimensions (w-[280px] h-[658px]) on medium screens and above is a solid approach.
223-223: LGTM! Good padding adjustment for mobile view.
The padding adjustment (px-2 md:px-8) provides better space utilization on mobile while maintaining comfortable spacing on larger screens.
platform/src/pages/analytics/_components/OverView.jsx (1)
113-113: LGTM!
The refetch call is appropriately placed within the handleDateChange callback to refresh data when the date range changes.
platform/src/common/components/Modal/dataDownload/modules/MoreInsights.jsx (2)
Line range hint 1-297: Verify intentional removal of features
Several features appear to be commented out (download button, add location button). Please confirm if this is intentional and aligns with the PR objectives.
#!/bin/bash
# Search for references to these features in other files
echo "Searching for download button references..."
rg "downloadData|DownloadIcon" --type jsx
echo "Searching for add location button references..."
rg "moreSights|Add location" --type jsx212-212: Verify calendar dropdown positioning on small screens
The negative left offset (-left-10) might cause horizontal overflow on very small screens.
✅ Verification successful
Re-run search without specifying file type to verify negative positioning patterns
Extract specific instances of negative left positioning for detailed review
| <Button | ||
| type="button" | ||
| variant="filled" | ||
| onClick={handleSubmit} | ||
| disabled={loading} // Disable button when loading | ||
| disabled={loading} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider disabling both buttons during loading
Currently, both the primary action and cancel buttons are disabled during loading, which might frustrate users who want to cancel a long-running operation.
Consider keeping the cancel button enabled:
- disabled={loading}Also applies to: 69-69
| @@ -20,7 +20,7 @@ | |||
| }, [errorMessage, setError]); | |||
|
|
|||
| return ( | |||
| <div className="bg-gray-50 absolute bottom-0 right-0 w-full px-4 py-3 sm:px-6 flex items-center justify-between"> | |||
| <div className="bg-gray-50 absolute bottom-0 right-0 w-full px-4 py-3 sm:px-6 flex flex-col md:flex-row items-start md:items-center gap-2 justify-between"> | |||
There was a problem hiding this comment.
💡 Codebase verification
Absolute positioning in Footer.jsx may cause layout issues
The use of absolute positioning with w-full can lead to layout problems in various modal sizes or when content overflows. Consider adopting a more flexible layout approach.
🔗 Analysis chain
Consider potential layout issues with absolute positioning
The absolute positioning with w-full might cause layout issues in certain modal sizes or when content overflows. Consider using a more flexible layout approach.
- <div className="bg-gray-50 absolute bottom-0 right-0 w-full px-4 py-3 sm:px-6 flex flex-col md:flex-row items-start md:items-center gap-2 justify-between">
+ <div className="bg-gray-50 sticky bottom-0 w-full px-4 py-3 sm:px-6 flex flex-col md:flex-row items-start md:items-center gap-2 justify-between">Further verification needed for absolute positioning in Footer.jsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the modal has a fixed width that might conflict
rg -A 2 "Modal.*className.*w-\[380px\]"
Length of output: 41
Script:
# Searching for the usage of 'absolute' in Footer.jsx to assess potential layout issues
rg "absolute" platform/src/common/components/Modal/dataDownload/components/Footer.jsx
Length of output: 245
| console.info(chartLoading); | ||
|
|
There was a problem hiding this comment.
Remove console.info statement before production deployment.
This debugging statement should be removed as it serves no purpose in production and could potentially expose internal state information.
Apply this diff to remove the console statement:
- console.info(chartLoading);📝 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.
| console.info(chartLoading); |
| // State for managing the SkeletonLoader visibility | ||
| const [showSkeleton, setShowSkeleton] = useState(chartLoading); | ||
|
|
||
| /** | ||
| * Handles refetching data when a parameter changes. | ||
| */ | ||
| const handleParameterChange = useCallback(() => { | ||
| refetch(); | ||
| }, [refetch]); | ||
|
|
||
| /** | ||
| * Effect to manage SkeletonLoader visibility with delay | ||
| */ | ||
| useEffect(() => { | ||
| let timer; | ||
| if (chartLoading) { | ||
| setShowSkeleton(true); | ||
| } else { | ||
| timer = setTimeout(() => { | ||
| setShowSkeleton(false); | ||
| }, 8000); | ||
| } | ||
|
|
||
| return () => { | ||
| if (timer) clearTimeout(timer); | ||
| }; | ||
| }, [chartLoading]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the skeleton loader timing logic
The current implementation has a fixed 8-second delay which might not be optimal for all scenarios. This could lead to unnecessary skeleton display even when data loads quickly.
Consider this more responsive approach:
- const [showSkeleton, setShowSkeleton] = useState(chartLoading);
+ const [showSkeleton, setShowSkeleton] = useState(false);
+ const MINIMUM_LOADER_TIME = 500; // 500ms minimum to avoid flashing
useEffect(() => {
let timer;
if (chartLoading) {
setShowSkeleton(true);
} else {
- timer = setTimeout(() => {
- setShowSkeleton(false);
- }, 8000);
+ // Only add delay if skeleton was actually shown
+ if (showSkeleton) {
+ timer = setTimeout(() => {
+ setShowSkeleton(false);
+ }, MINIMUM_LOADER_TIME);
+ }
}
return () => {
if (timer) clearTimeout(timer);
};
- }, [chartLoading]);
+ }, [chartLoading, showSkeleton]);📝 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.
| // State for managing the SkeletonLoader visibility | |
| const [showSkeleton, setShowSkeleton] = useState(chartLoading); | |
| /** | |
| * Handles refetching data when a parameter changes. | |
| */ | |
| const handleParameterChange = useCallback(() => { | |
| refetch(); | |
| }, [refetch]); | |
| /** | |
| * Effect to manage SkeletonLoader visibility with delay | |
| */ | |
| useEffect(() => { | |
| let timer; | |
| if (chartLoading) { | |
| setShowSkeleton(true); | |
| } else { | |
| timer = setTimeout(() => { | |
| setShowSkeleton(false); | |
| }, 8000); | |
| } | |
| return () => { | |
| if (timer) clearTimeout(timer); | |
| }; | |
| }, [chartLoading]); | |
| // State for managing the SkeletonLoader visibility | |
| const [showSkeleton, setShowSkeleton] = useState(false); | |
| const MINIMUM_LOADER_TIME = 500; // 500ms minimum to avoid flashing | |
| /** | |
| * Handles refetching data when a parameter changes. | |
| */ | |
| const handleParameterChange = useCallback(() => { | |
| refetch(); | |
| }, [refetch]); | |
| /** | |
| * Effect to manage SkeletonLoader visibility with delay | |
| */ | |
| useEffect(() => { | |
| let timer; | |
| if (chartLoading) { | |
| setShowSkeleton(true); | |
| } else { | |
| // Only add delay if skeleton was actually shown | |
| if (showSkeleton) { | |
| timer = setTimeout(() => { | |
| setShowSkeleton(false); | |
| }, MINIMUM_LOADER_TIME); | |
| } | |
| } | |
| return () => { | |
| if (timer) clearTimeout(timer); | |
| }; | |
| }, [chartLoading, showSkeleton]); |
|
New next-platform changes available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
platform/src/common/components/Charts/MoreInsightsChart.jsx (2)
320-320: Performance optimization with React.memo 🚀Good addition of React.memo to prevent unnecessary re-renders. However, for complex props, consider adding a custom comparison function.
Consider adding a custom comparison function to optimize re-renders further:
export default React.memo(MoreInsightsChart, (prevProps, nextProps) => { // Compare only the props that affect visual output return ( prevProps.data === nextProps.data && prevProps.selectedSites === nextProps.selectedSites && prevProps.chartType === nextProps.chartType && prevProps.pollutantType === nextProps.pollutantType ); });
Line range hint
1-320: Consider implementing error boundaries for chart renderingThe component handles data validation well but could benefit from error boundaries to gracefully handle runtime errors in chart rendering.
Consider implementing an error boundary component:
class ChartErrorBoundary extends React.Component { state = { hasError: false }; static getDerivedStateFromError(error) { return { hasError: true }; } render() { if (this.state.hasError) { return <div>Something went wrong with the chart. Please try refreshing.</div>; } return this.props.children; } } // Usage: export default React.memo((props) => ( <ChartErrorBoundary> <MoreInsightsChart {...props} /> </ChartErrorBoundary> ));platform/src/common/components/Charts/ChartContainer.jsx (2)
200-214: UseuseMemoinstead ofuseCallbackfor memoizing components likeErrorOverlay.Since
ErrorOverlayreturns JSX without accepting any arguments,useMemois more suitable for memoizing this component thanuseCallback.
Line range hint
151-193: Review dependencies and memoization inrenderDropdownContent.Ensure all dependencies used within
renderDropdownContentare correctly specified. Additionally, confirm that functions likehandleRefreshChartandhandleOpenModalare memoized to prevent unnecessary re-renders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
platform/src/common/components/Charts/ChartContainer.jsx(7 hunks)platform/src/common/components/Charts/MoreInsightsChart.jsx(3 hunks)platform/src/core/utils/useFetchAnalyticsData.jsx(3 hunks)platform/src/pages/analytics/_components/OverView.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- platform/src/core/utils/useFetchAnalyticsData.jsx
- platform/src/pages/analytics/_components/OverView.jsx
🔇 Additional comments (3)
platform/src/common/components/Charts/MoreInsightsChart.jsx (1)
59-61: Improved data validation with early returns 👍
The early return pattern for invalid data points enhances code readability and maintainability. This change aligns well with best practices by reducing nesting levels.
platform/src/common/components/Charts/ChartContainer.jsx (2)
62-74: Verify that the timer in useEffect is managed correctly.
Double-check that the timer is properly cleared to prevent potential memory leaks or unintended behavior, especially if chartLoading changes rapidly.
264-264: Great use of React.memo to optimize ChartContainer.
Wrapping ChartContainer with React.memo enhances performance by preventing unnecessary re-renders when props haven't changed.
| @@ -309,6 +307,7 @@ const MoreInsightsChart = ({ | |||
| renderCustomizedLegend, | |||
| frequency, | |||
| siteIdToName, | |||
| refreshChart, | |||
There was a problem hiding this comment.
💡 Codebase verification
Stabilize refreshChart with useCallback to prevent unnecessary re-renders.
refreshChartis not currently memoized, which may lead to unnecessary re-renders of the component.
🔗 Analysis chain
Verify the necessity of refreshChart in dependency array
While adding refreshChart to the dependency array is correct as it's used within the useMemo hook, we should ensure that refreshChart is stable (e.g., wrapped in useCallback) to prevent unnecessary re-renders.
Consider wrapping the refreshChart callback in useCallback at the parent component if it isn't already:
const refreshChart = useCallback(() => {
// refresh logic
}, [/* dependencies */]);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition of refreshChart prop to verify if it's properly memoized
ast-grep --pattern 'const $_ = useCallback($$$)' | rg 'refreshChart'
Length of output: 68
Script:
#!/bin/bash
# Search for all instances of refreshChart and include 2 lines of surrounding context
rg -C 2 'refreshChart'
Length of output: 2868
| useOutsideClick(dropdownRef, () => { | ||
| dropdownRef.current?.classList.remove('show'); | ||
| setDownloadComplete(null); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using state instead of directly manipulating classList.
Directly modifying the DOM with classList.remove('show') isn't aligned with React's best practices. Using state to control the visibility of the dropdown ensures consistent and predictable UI updates within React's rendering flow.
| @@ -154,38 +177,41 @@ | |||
| ))} | |||
| <hr className="border-gray-200" /> | |||
| <button | |||
| onClick={() => handleOpenModal('inSights', [], user_selected_sites)} | |||
| onClick={() => handleOpenModal('inSights', [], userSelectedSites)} | |||
There was a problem hiding this comment.
Possible typo: 'inSights' should be 'insights' in handleOpenModal.
It appears that 'inSights' might be a typo. Correcting it to 'insights' will ensure the modal functions as intended.
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
Release Notes
New Features
SkeletonLoaderin multiple components for better user feedback during data fetching.DataDownloadcomponent, ensuring appropriate error messages for invalid data formats.Improvements
ChartContainercomponent.Bug Fixes
FooterandMoreInsightscomponents to enhance clarity and responsiveness.