Center align Loading component based on remaining screen height
#8976
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@Suhail2701 keep readable titles for the PR and fo follow the PR template which includes the merge checklist. |
|
Hi @rithviknishad, I have updated the Loading component as you mentioned. Kindly check it and let me know your feedback. Thank you! |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in the Changes
Assessment against linked issues
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/components/Common/Loading.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@typescript-eslint/parser' declared in '.eslintrc.json': Cannot find module '@typescript-eslint/parser'
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
src/components/Common/Loading.tsx (1)
8-8: Consider using responsive width classes.The fixed
w-2/12width might not provide the best experience across all screen sizes. Consider using responsive width classes or max-width constraints.- <div className="w-2/12"> + <div className="w-2/12 sm:w-3/12 md:w-2/12 max-w-xs">src/components/Facility/FacilityList.tsx (1)
113-113: Consider making the loader height responsive.While setting
minHeightClass="min-h-[600px]"helps center the loader, consider making it responsive for better mobile experience:- manageFacilities = <Loading minHeightClass={"min-h-[600px]"} />; + manageFacilities = <Loading minHeightClass={"min-h-[300px] md:min-h-[450px] lg:min-h-[600px]"} />;src/components/Patient/SampleViewAdmin.tsx (1)
287-289: Consider using viewport-based height for better responsiveness.While the fixed
min-h-[600px]works well for standard viewports, consider using a viewport-based height for better responsiveness across different screen sizes.- <Loading minHeightClass={"min-h-[600px]"} /> + <Loading minHeightClass={"min-h-[60vh]"} />src/components/Assets/AssetsList.tsx (2)
212-212: Consider standardizing the minimum height value across components.The
minHeightClassvalue of "min-h-[500px]" differs from the "min-h-[600px]" used in other components (FacilityList, ManagePatients, etc.). Consider standardizing this value for consistent loading state appearance across the application.- <Loading minHeightClass={"min-h-[500px]"} /> + <Loading minHeightClass={"min-h-[600px]"} />
Line range hint
156-189: Consolidate loading state management.The loading state is being reset in multiple places (try block, catch block, and finally block). This could lead to race conditions or state inconsistencies. Consider letting the finally block handle all loading state resets.
const accessAssetIdFromQR = async (assetURL: string) => { try { setIsLoading(true); setIsScannerActive(false); if (!isValidURL(assetURL)) { - setIsLoading(false); Notification.Error({ msg: t("invalid_asset_id_msg"), }); return; } const params = parseQueryParams(assetURL); // QR Maybe searchParams "asset" or "assetQR" // If no params found, then use assetText const assetId = params.asset || params.assetQR; if (assetId) { const { data } = await request(routes.listAssetQR, { pathParams: { qr_code_id: assetId }, }); if (!data) { - setIsLoading(false); Notification.Error({ msg: t("invalid_asset_id_msg"), }); return; } const { data: assetData } = await request(routes.listAssets, { query: { qr_code_id: assetId, limit: 1 }, }); if (assetData?.results.length === 1) { navigate( `/facility/${assetData.results[0].location_object.facility?.id}/assets/${assetData.results[0].id}`, ); } else { - setIsLoading(false); Notification.Error({ msg: t("asset_not_found_msg"), }); } } else { - setIsLoading(false); Notification.Error({ msg: t("invalid_asset_id_msg"), }); } } catch (err) { Notification.Error({ msg: t("invalid_asset_id_msg"), }); } finally { setIsLoading(false); } };src/components/Users/ManageUsers.tsx (2)
469-469: Consider making the min-height value more flexible.While the
minHeightClassprop helps position the loader, the hardcoded value of "min-h-[600px]" might not be optimal for all viewport sizes.Consider one of these approaches:
- Make it responsive using viewport units:
-manageUsers = <Loading minHeightClass={"min-h-[600px]"} />; +manageUsers = <Loading minHeightClass={"min-h-[60vh]"} />;
- Pass it as a prop from the parent to allow for context-specific heights:
-manageUsers = <Loading minHeightClass={"min-h-[600px]"} />; +manageUsers = <Loading minHeightClass={loadingMinHeightClass} />;
Line range hint
1-1000: Consider breaking down the component for better maintainability.The
ManageUserscomponent handles multiple responsibilities including user listing, filtering, facility management, and working hours management. This could make it harder to maintain and test.Consider these architectural improvements:
- Extract facility management logic into a separate service/hook:
// hooks/useFacilityManagement.ts export function useFacilityManagement(username: string) { const updateHomeFacility = async (facility: any) => { // ... facility update logic }; const unlinkFacility = async (facility: any) => { // ... facility unlink logic }; return { updateHomeFacility, unlinkFacility }; }
- Create a custom hook for user filtering logic:
// hooks/useUserFilters.ts export function useUserFilters() { // ... filtering logic return { filters, updateFilters }; }
- Split the
UserFacilitiescomponent into its own file since it's a significant piece of functionality.These changes would improve code organization, reusability, and testability.
src/components/Patient/ManagePatients.tsx (1)
757-757: Consider using responsive units forminHeightClassThe hardcoded
min-h-[500px]may not display optimally on all screen sizes, especially smaller devices. To enhance responsiveness, consider using Tailwind CSS responsive classes to adjust the minimum height based on screen size.Apply this diff to make the
minHeightClassresponsive:-<Loading minHeightClass={"min-h-[500px]"} /> +<Loading minHeightClass={"min-h-screen md:min-h-[500px]"} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
src/components/Assets/AssetsList.tsx(1 hunks)src/components/Common/Loading.tsx(1 hunks)src/components/Facility/FacilityList.tsx(1 hunks)src/components/Patient/ManagePatients.tsx(1 hunks)src/components/Patient/SampleViewAdmin.tsx(1 hunks)src/components/Users/ManageUsers.tsx(1 hunks)
🔇 Additional comments (3)
src/components/Common/Loading.tsx (2)
9-9: Consider replacing App-logo CSS class with Tailwind.
In line with the previous review comment about mixing CSS and Tailwind, consider replacing the App-logo CSS class with equivalent Tailwind classes if it's only handling simple styling.
Let's check the App-logo CSS definition:
#!/bin/bash
# Description: Find App-logo CSS definition
# Expected: CSS properties that could be replaced with Tailwind
# Search for App-logo class definition
rg 'App-logo' --type css3-7: LGTM! The loader positioning implementation looks good.
The changes effectively address the centering requirements by:
- Using flex layout with proper centering classes
- Making the height configurable via props
Let's verify the loader usage across different pages:
✅ Verification successful
The Loading component implementation and usage patterns are verified as appropriate
The verification shows:
- The component is used consistently across multiple pages with proper minHeightClass customization:
- Default
min-h-screenworks for full-page loads - Custom heights like
min-h-[500px]andmin-h-[600px]are used appropriately in list views
- Default
- The App-logo CSS is properly defined with:
- Responsive height (10vmin)
- Proper animation for the loading state
- Consistent styling across the app
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Loading component usage patterns
# Expected: Various minHeightClass values across different pages
# Search for Loading component usage
rg -A 2 '<Loading' --type tsx
Length of output: 308
Script:
#!/bin/bash
# Let's try different search patterns to find Loading component usage
# Search for Loading import statements
rg -l "import.*Loading.*from.*Common" --type tsx
# Search for Loading component with word boundaries
rg -A 2 "\bLoading\b.*>" --type tsx
# Search for files containing Loading component
fd -e tsx -x grep -l "Loading"
# Check the actual component implementation
cat src/components/Common/Loading.tsx
Length of output: 6020
Script:
#!/bin/bash
# Let's check the actual usage patterns of the Loading component
# Check a few key files that might show different usage patterns
echo "=== AssetsList.tsx ==="
rg -A 3 -B 1 '<Loading' src/components/Assets/AssetsList.tsx || true
echo "=== UserProfile.tsx ==="
rg -A 3 -B 1 '<Loading' src/components/Users/UserProfile.tsx || true
echo "=== ResourceList.tsx ==="
rg -A 3 -B 1 '<Loading' src/components/Resource/ResourceList.tsx || true
# Check for any custom minHeightClass props
echo "=== Custom minHeightClass usage ==="
rg '<Loading.*minHeightClass' . || true
# Check the CSS file for App-logo styles
echo "=== App-logo CSS ==="
cat src/style/index.css || true
Length of output: 25868
src/components/Patient/SampleViewAdmin.tsx (1)
287-289: LGTM! Effective centering solution implemented.
The combination of flex container with justify-center and the explicit min-h-[600px] ensures proper vertical centering of the loader, addressing the positioning issue described in #8762.
| @@ -110,7 +110,7 @@ export const FacilityList = () => { | |||
| } | |||
|
|
|||
| if (isLoading || !permittedData) { | |||
| manageFacilities = <Loading />; | |||
| manageFacilities = <Loading minHeightClass={"min-h-[600px]"} />; | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can generate the final response as I have sufficient information about the Loading component usage across the codebase:
Height inconsistency detected in Loading component usage
The Loading component is used with different height configurations across the codebase:
min-h-[600px]in FacilityList.tsx and ManageUsers.tsxmin-h-[500px]in ManagePatients.tsx and AssetsList.tsx- Most other occurrences have no height specification
Consider standardizing the loading height:
- Either remove the custom height from FacilityList.tsx to match majority usage
- Or establish a consistent height standard across all list views (ManageUsers, ManagePatients, AssetsList)
🔗 Analysis chain
Verify consistent loader height across components.
Let's ensure this height value is consistently applied across other components using the Loading component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Loading component usages to verify consistent height values
rg -A 1 '<Loading' | grep -v 'FacilityList.tsx'
Length of output: 10138
|
👋 Hi, @Suhail2701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
…lign based on remaining screen height
rithviknishad
changed the title
Loading component based on remaining screen height
rithviknishad
added
needs testing
needs review
and removed
invalid
rithviknishad
requested review from
bodhish,
shivankacker,
Jacobjeevan and
khavinshankar
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Common/Loading.tsx (2)
7-24: Optimize event listeners for better performanceThe current implementation could cause frequent recalculations. Consider these optimizations:
- Add debounce to the resize and scroll handlers
- Remove capture phase from scroll listener unless specifically needed
Consider this implementation:
+import { debounce } from 'lodash'; + useEffect(() => { - const calculateOffset = () => { + const calculateOffset = debounce(() => { if (containerRef.current) { const rect = containerRef.current.getBoundingClientRect(); setOffsetTop(rect.top); } - }; + }, 100); calculateOffset(); window.addEventListener("resize", calculateOffset); - window.addEventListener("scroll", calculateOffset, true); + window.addEventListener("scroll", calculateOffset); return () => { window.removeEventListener("resize", calculateOffset); - window.removeEventListener("scroll", calculateOffset, true); + window.removeEventListener("scroll", calculateOffset); + calculateOffset.cancel(); }; }, []);
32-37: Make logo width responsiveUsing a fixed width ratio (w-2/12) might not work well across all screen sizes. Consider using responsive classes or max-width constraints.
Apply this diff:
-<div className="w-2/12"> +<div className="w-32 sm:w-40 md:w-48 lg:w-56 max-w-[16.666667%]">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Common/Loading.tsx(1 hunks)
🔇 Additional comments (3)
src/components/Common/Loading.tsx (3)
1-6: 🛠️ Refactor suggestion
Add className prop for better component reusability
The component should accept a className prop to allow customization from parent components, as suggested in previous reviews.
Apply this diff:
-const Loading = () => {
+const Loading = (props: { className?: string }) => {
const containerRef = useRef<HTMLDivElement>(null);
const [offsetTop, setOffsetTop] = useState(0);27-31: 🛠️ Refactor suggestion
Use className prop and avoid mixing styles
- The className prop should be used with the container div
- Avoid mixing inline styles with Tailwind when possible
Apply this diff:
<div
ref={containerRef}
- className="flex w-full items-center justify-center transition-[height]"
- style={{ height: `calc(100vh - ${offsetTop}px)` }}
+ className={cn(
+ "flex w-full items-center justify-center transition-[height]",
+ `h-[calc(100vh-${offsetTop}px)]`,
+ props.className
+ )}
>1-42: Verify loader positioning across different scenarios
While the implementation should center the loader correctly, please verify these scenarios:
- Different page layouts and tab switches
- Various screen sizes
- Dynamic content changes that might affect positioning
Run this script to find all usage locations:
✅ Verification successful
Based on the search results, I can see that the Loading component is used extensively throughout the application in various scenarios like data fetching, form submissions, and route transitions. The component is consistently used as a full-page loader with the height calculation logic to ensure proper centering.
Let me generate the final response:
Loading component implementation looks correct and well-used
The dynamic height calculation implementation in the Loading component is appropriate and works well with the application's needs:
- Used consistently across 50+ components as a full-page loader
- Handles various scenarios including Suspense fallbacks, data loading states, and form submissions
- The height calculation ensures proper vertical centering regardless of the page layout or header height
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all files that import and use the Loading component
rg -l "import.*Loading.*from.*Common/Loading" | xargs rg "Loading"
Length of output: 31913
|
LGTM |
|
@Suhail2701 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
…cnetwork#8976) Co-authored-by: rithviknishad <[email protected]>
Fixes #8762 - Repositions the page loader to be centered on the all pages.
This PR updates the loader component’s CSS, ensuring that it appears in the center of the screen on all pages.
After Changes:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor