Fix: Move action buttons to sticky footer in AddChargeItemsBillingSheet #16099#16118
Fix: Move action buttons to sticky footer in AddChargeItemsBillingSheet #16099#16118niyati10000 wants to merge 1 commit intoohcnetwork:developfrom
Conversation
|
WalkthroughThe AddChargeItemsBillingSheet component was refactored to introduce a persistent footer area containing action buttons. Action buttons were relocated from inside the ScrollArea to a SheetFooter component positioned as a fixed, sticky footer. ScrollArea padding was adjusted accordingly, and button layout was simplified while preserving disabled states and shortcut badge behavior. Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
Greptile SummaryThis PR fixes a UX issue where the "Cancel" and "Add Items" action buttons were buried inside the The overall approach is sound. A few small styling issues are worth addressing:
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 750da78 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| </div> | ||
| </ScrollArea> | ||
| {/* Fixed Footer with Action Buttons */} | ||
| <SheetFooter className="p-4 border-t bg-white sticky bottom-0 flex-row justify-end gap-2"> |
There was a problem hiding this comment.
Double spacing on sm screens from gap-2 + sm:space-x-2
SheetFooter applies sm:space-x-2 by default (via cn("flex flex-col-reverse sm:flex-row sm:justify-end sm:space-x-2", className) in sheet.tsx). Since tailwind-merge does not treat gap-* and space-x-* as conflicting utilities (they produce different CSS properties), both will be applied simultaneously on sm and larger breakpoints — resulting in ~8px of margin-left from space-x-2 stacked on top of the 8px gap-2, giving roughly 16px of total spacing between the two buttons instead of the intended 8px.
To fix this, either negate the default sm:space-x-2 or don't add gap-2 here and instead rely on the default:
| <SheetFooter className="p-4 border-t bg-white sticky bottom-0 flex-row justify-end gap-2"> | |
| <SheetFooter className="p-4 border-t bg-white sticky bottom-0 flex-row justify-end gap-2 sm:space-x-0"> |
| </div> | ||
| </ScrollArea> | ||
| {/* Fixed Footer with Action Buttons */} | ||
| <SheetFooter className="p-4 border-t bg-white sticky bottom-0 flex-row justify-end gap-2"> |
There was a problem hiding this comment.
sticky bottom-0 is redundant in this layout
SheetContent is already a fixed-position element with flex flex-col and h-full (from inset-y-0). ScrollArea has flex-1, so it consumes all remaining vertical space — the SheetFooter naturally sits at the bottom of the visible panel as the next flex sibling, with no ability to scroll past it. The sticky bottom-0 class has no practical effect here and may be misleading.
Consider removing sticky bottom-0 to keep the intent clear:
| <SheetFooter className="p-4 border-t bg-white sticky bottom-0 flex-row justify-end gap-2"> | |
| <SheetFooter className="p-4 border-t bg-white flex-row justify-end gap-2 sm:space-x-0"> |
| </div> | ||
| </ScrollArea> | ||
| {/* Fixed Footer with Action Buttons */} | ||
| <SheetFooter className="p-4 border-t bg-white sticky bottom-0 flex-row justify-end gap-2"> |
There was a problem hiding this comment.
Missing dark mode background on footer
bg-white is hardcoded without a dark-mode counterpart. The SheetContent base styles include dark:bg-gray-950, so on dark mode the footer background will visually mismatch the panel. Consider pairing it with a dark mode class:
| <SheetFooter className="p-4 border-t bg-white sticky bottom-0 flex-row justify-end gap-2"> | |
| <SheetFooter className="p-4 border-t bg-white dark:bg-gray-950 sticky bottom-0 flex-row justify-end gap-2 sm:space-x-0"> |
🚀 Preview Deployment Ready!🔗 Preview URL: https://pr-16118.care-preview-a7w.pages.dev 📱 Mobile Access: This preview will be automatically updated when you push new commits to this PR. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/pages/Facility/billing/account/components/AddChargeItemsBillingSheet.tsx`:
- Line 378: The SheetFooter’s className is forcing desktop-only layout and a
hardcoded color; remove the literal "flex-row" and "bg-white" from the
SheetFooter className in AddChargeItemsBillingSheet so the component retains its
mobile-first stacked behavior and theme/contrast support, replace "flex-row"
with a responsive class like "sm:flex-row" (or "md:flex-row" if preferred) to
enable horizontal layout only on larger screens, and swap "bg-white" for a
semantic background token used in the app (e.g., "bg-surface" or
"bg-background") to preserve theme and high-contrast modes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8f19f22-f58b-4d32-afff-541b169041d3
📒 Files selected for processing (1)
src/pages/Facility/billing/account/components/AddChargeItemsBillingSheet.tsx
| </div> | ||
| </ScrollArea> | ||
| {/* Fixed Footer with Action Buttons */} | ||
| <SheetFooter className="p-4 border-t bg-white sticky bottom-0 flex-row justify-end gap-2"> |
There was a problem hiding this comment.
Avoid overriding footer responsiveness and hardcoded background on Line 378.
flex-row overrides SheetFooter’s mobile-friendly default stacking, and bg-white can break theme/high-contrast behavior. Keep responsive defaults and use semantic background tokens.
Proposed fix
- <SheetFooter className="p-4 border-t bg-white sticky bottom-0 flex-row justify-end gap-2">
+ <SheetFooter className="sticky bottom-0 border-t bg-background p-4 gap-2">As per coding guidelines, "Use Tailwind's responsive classes and follow mobile-first approach" and "Include high contrast support for visibility in various clinical lighting conditions."
📝 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.
| <SheetFooter className="p-4 border-t bg-white sticky bottom-0 flex-row justify-end gap-2"> | |
| <SheetFooter className="sticky bottom-0 border-t bg-background p-4 gap-2"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Facility/billing/account/components/AddChargeItemsBillingSheet.tsx`
at line 378, The SheetFooter’s className is forcing desktop-only layout and a
hardcoded color; remove the literal "flex-row" and "bg-white" from the
SheetFooter className in AddChargeItemsBillingSheet so the component retains its
mobile-first stacked behavior and theme/contrast support, replace "flex-row"
with a responsive class like "sm:flex-row" (or "md:flex-row" if preferred) to
enable horizontal layout only on larger screens, and swap "bg-white" for a
semantic background token used in the app (e.g., "bg-surface" or
"bg-background") to preserve theme and high-contrast modes.
|
Please do not open a PR until issue is assigned to you. |
|
Hi @maintainers, I went through the previous PR (#16118) and the related discussions. The approach of moving the action buttons into a SheetFooter makes sense and correctly addresses the main issue. However, I noticed a few follow-up improvements based on the review comments:
I’d like to work on refining this implementation by addressing these issues while keeping the fix minimal and aligned with existing UI patterns. Please let me know if I can proceed with this improvement. |
3 participants
Proposed Changes
Fixes #16099
ScrollAreato a stickySheetFooter.Tagging: @ohcnetwork/care-fe-code-reviewers
[x] Complete QA on desktop devices.
[x] Request peer reviews.
Summary by CodeRabbit