Improved Button Responsiveness and Layout Adjustments #9161
Conversation
fixed responsiveness of the website with small changes
WalkthroughThe changes in this pull request focus on modifying the layout and styling of various UI components across multiple files. Key updates include enhancements to margin and spacing for buttons and containers, adjustments to the rendering logic of tooltips, and refinements in the handling of state variables related to user interactions. Additionally, the control flow for managing patient consent records has been improved to ensure better user experience and data integrity. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (1.23.1)
src/components/Common/ButtonV2.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'
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
src/components/Kanban/Board.tsx (3)
61-61: Improve class naming conventionWhile the addition of flex-wrap styling for responsive design is good, the class name
containerssappears to have a typo and doesn't follow clear naming conventions.Consider renaming to something more semantic like:
-<div className="flex items-stretch px-0 pb-2 containerss"> +<div className="flex items-stretch px-0 pb-2 kanban-sections-container">
149-149: Improve CSS class organization and namingThe className string is becoming long and mixes utility classes with custom classes. The
bshadowclass name is not descriptive enough.Consider:
- Using a more descriptive name like
kanban-section-shadow- Organizing classes using a CSS-in-JS solution or CSS modules
- Creating a dedicated class that encapsulates all these styles
-"relative mr-2 w-[300px] shrink-0 rounded-xl bg-secondary-200 bshadow" +"kanban-section relative mr-2 w-[300px] shrink-0 rounded-xl bg-secondary-200"Then define the styles in your CSS:
.kanban-section { box-shadow: /* your shadow values */; }
Line range hint
108-117: Consider performance and type safety improvementsThe scroll event handler could benefit from debouncing, and the response type could be more strictly typed.
Consider these improvements:
- Debounce the scroll event handler:
import { debounce } from 'lodash'; const debouncedOnBoardReachEnd = debounce(async () => { // existing logic }, 150); useEffect(() => { props.boardRef.current?.addEventListener("scroll", debouncedOnBoardReachEnd); return () => { props.boardRef.current?.removeEventListener("scroll", debouncedOnBoardReachEnd); debouncedOnBoardReachEnd.cancel(); }; }, [props.boardRef, fetchingNextPage, hasMore]);
- Add proper typing for the API response:
interface PaginatedResponse<T> { results: T[]; next: string | null; count: number; }src/components/Common/ButtonV2.tsx (1)
104-104: Reconsider adding global margin to base button stylesAdding
mt-2to the base button styles will affect all instances of ButtonV2 across the application. This could potentially break existing layouts and violates the principle of separation of concerns, as spacing should typically be controlled by the parent component.Consider these alternatives:
- Add the margin where specifically needed using the
classNameprop- Create a new variant or prop for buttons that need this spacing
- Handle the spacing in the parent component's layout
Example of better approaches:
// Option 1: Add margin only where needed <ButtonV2 className="mt-2" /> // Option 2: Add a prop for spacing type ButtonProps = { // ... existing props spacing?: 'none' | 'compact' | 'normal' | 'loose'; } // Option 3: Handle in parent (preferred) <div className="space-y-2"> <ButtonV2 /> <ButtonV2 /> </div>src/components/Patient/PatientConsentRecords.tsx (1)
235-235: LGTM! Consider using CSS modules for better maintainability.The addition of
mt-2class improves vertical spacing consistency. However, consider using CSS modules or styled-components to better manage these utility classes and improve maintainability.- "button-size-default button-shape-square button-primary-default inline-flex mt-2 h-min w-full cursor-pointer items-center justify-center gap-2 whitespace-pre font-medium outline-offset-1 transition-all duration-200 ease-in-out" + styles.uploadButton // Define in a separate CSS modulesrc/components/Facility/FacilityCard.tsx (1)
Line range hint
1-276: Consider these architectural improvements for better maintainability.While the current changes improve the layout, here are some suggestions to enhance the component's maintainability:
- Consider extracting the notification modal into a separate component to reduce complexity
- The responsive layout logic could be simplified by using Tailwind's
@screendirectives in a separate CSS file- Common class combinations could be extracted into reusable Tailwind components
Example of extracting the notification modal:
// NotificationModal.tsx interface NotificationModalProps { facilityId: string; facilityName: string; isOpen: boolean; onClose: () => void; onSubmit: (message: string) => void; } const NotificationModal: React.FC<NotificationModalProps> = ({ facilityId, facilityName, isOpen, onClose, onSubmit }) => { const [message, setMessage] = useState(""); const [error, setError] = useState(""); // ... rest of the modal logic };src/style/index.css (2)
1319-1323: Consider using a more conventional class nameThe class name
containersswith double 's' appears to be a typo or non-conventional naming. Consider renaming it to something more semantic likeresponsive-containeror simplycontainer.- .containerss{ + .responsive-container { flex-wrap: wrap; gap: 5px; margin-bottom: 5px; }
1324-1327: Remove commented code and use a more descriptive class name
- The commented-out box-shadow property should either be implemented or removed.
- Consider using a more descriptive class name than
bshadowto better convey its purpose.- .bshadow{ - border:solid rgb(168, 168, 168); - /* box-shadow: 3px solid rgb(0, 0, 0) !important; */ - } + .bordered-container { + border: solid rgb(168, 168, 168); + }src/components/Patient/ManagePatients.tsx (1)
844-844: LGTM! Consider extracting the className into a constant.The addition of
flex-wrapimproves the layout handling of filter controls, especially on smaller screens. However, for better maintainability, consider extracting the long className string into a constant or using a CSS module.+ const filterControlsClassName = "flex w-full flex-col items-center justify-end flex-wrap gap-2 lg:ml-3 lg:w-fit lg:flex-row lg:gap-3"; - <div className="flex w-full flex-col items-center justify-end flex-wrap gap-2 lg:ml-3 lg:w-fit lg:flex-row lg:gap-3"> + <div className={filterControlsClassName}>
📜 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 (8)
src/CAREUI/interactive/FiltersSlideover.tsx(1 hunks)src/components/Common/ButtonV2.tsx(1 hunks)src/components/Common/Menu.tsx(1 hunks)src/components/Facility/FacilityCard.tsx(2 hunks)src/components/Kanban/Board.tsx(2 hunks)src/components/Patient/ManagePatients.tsx(1 hunks)src/components/Patient/PatientConsentRecords.tsx(1 hunks)src/style/index.css(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/CAREUI/interactive/FiltersSlideover.tsx
- src/components/Common/Menu.tsx
🧰 Additional context used
🪛 Biome
src/style/index.css
[error] 1328-1328: expected } but instead the file ends
the file ends here
(parse)
🔇 Additional comments (2)
src/components/Facility/FacilityCard.tsx (2)
91-91: LGTM! Improved spacing between flex items.
The addition of gap-2 creates consistent spacing between items in the facility name section, improving the visual layout.
223-223: LGTM! Enhanced button alignment.
The addition of justify-center improves the visual alignment of the action buttons, particularly on smaller screens where the container might wrap.
src/components/Common/ButtonV2.tsx
Outdated
| @@ -101,7 +101,7 @@ export const buttonStyles = ({ | |||
| shadow = !ghost, | |||
| }: ButtonStyleProps) => { | |||
| return classNames( | |||
| "inline-flex h-min cursor-pointer items-center justify-center gap-2 whitespace-pre font-medium outline-offset-1 transition-all duration-200 ease-in-out disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500", | |||
| "inline-flex h-min cursor-pointer items-center justify-center gap-2 whitespace-pre mt-2 font-medium outline-offset-1 transition-all duration-200 ease-in-out disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500", | |||
There was a problem hiding this comment.
💡 Codebase verification
Adding default margin-top could cause layout inconsistencies
The mt-2 class has been added to the base button styles which could cause unexpected spacing in existing implementations. Based on the evidence:
- The ButtonV2 component is widely used across 100+ files in the codebase
- Current implementations use explicit className props for spacing when needed (e.g.,
m-2,py-[11px]) - The default margin-top in the base styles could interfere with existing layouts where buttons are precisely positioned
Recommendation:
- Remove the
mt-2from the base button styles - Let consuming components control their own spacing through className props as they currently do
🔗 Analysis chain
Verify the impact on existing button instances
Let's assess how this change affects existing button implementations across the codebase.
The previous search showed widespread usage of ButtonV2 across the codebase. Now we need to examine actual usage patterns and the component definition to understand if the new margin will conflict with existing implementations.
The previous results show some ButtonV2 usages with custom classes, but we need to see the full component implementation and check specifically for margin-top conflicts to make a proper assessment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ButtonV2 usage patterns that might conflict with the new margin
echo "Checking for ButtonV2 instances with existing margin classes..."
rg -l "ButtonV2.*className.*mt-"
echo "Checking for all ButtonV2 usages to understand the scope of impact..."
rg -l "ButtonV2"
echo "Checking for potential negative margin overrides that might be compensating for spacing..."
rg -l "ButtonV2.*className.*-mt-"
Length of output: 6867
Script:
#!/bin/bash
# Let's check for actual ButtonV2 usage patterns with className to understand margin conflicts
echo "Checking ButtonV2 usages with className prop..."
rg "ButtonV2.*className.*" -A 1
# Let's also check the ButtonV2 component definition to understand its composition
echo "Checking ButtonV2 component definition..."
rg "const ButtonV2" -A 10 src/components/Common/ButtonV2.tsx
Length of output: 1040
Script:
#!/bin/bash
# Let's check the full ButtonV2 component implementation to understand the className composition
echo "Checking full ButtonV2 implementation..."
cat src/components/Common/ButtonV2.tsx
# Let's also check for any existing margin-top classes in ButtonV2 usages
echo "Checking for existing margin-top classes in ButtonV2 usages..."
rg "ButtonV2.*className.*mt-[0-9]" -B 1 -A 1
Length of output: 7373
There was a problem hiding this comment.
ButtonV2 is widely used - make sure this doesn't cause any issues elsewhere. Instead of modifying it here, modify wherever the specific issue is occurring.
There was a problem hiding this comment.
Understood, @Jacobjeevan. To prevent potential issues elsewhere, I've removed the mt-2 class from the ButtonV2 component. Instead, we can add the mt-2 class to the specific instances where the margin is needed.
For example, in src/CAREUI/interactive/FiltersSlideover.tsx, update the AdvancedFilterButton:
<ButtonV2 className="w-full bg-white md:w-auto mt-2" onClick={toggleSlideover}>
<CareIcon className="care-l-filter text-lg" />
<span>Advanced Filters</span>
</ButtonV2>This ensures that the margin is applied only where necessary without impacting other components.
✏️ Learnings added
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9161
File: src/components/Common/ButtonV2.tsx:104-104
Timestamp: 2024-11-20T05:14:50.931Z
Learning: When adjusting styling for widely used components like `ButtonV2`, avoid modifying the base styles to prevent unintended effects across the application. Instead, apply style changes directly in the specific instances where needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
src/style/index.css
Outdated
| @media screen and (max-width:915px){ | ||
| .containerss{ | ||
| flex-wrap: wrap; | ||
| gap: 5px; | ||
| margin-bottom: 5px; | ||
| } | ||
| .bshadow{ | ||
| border:solid rgb(168, 168, 168); | ||
| /* box-shadow: 3px solid rgb(0, 0, 0) !important; */ | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix syntax error: Add missing closing curly brace
The media query block is missing a closing curly brace at the end of the file.
@media screen and (max-width:915px){
.containerss{
flex-wrap: wrap;
gap: 5px;
margin-bottom: 5px;
}
.bshadow{
border:solid rgb(168, 168, 168);
/* box-shadow: 3px solid rgb(0, 0, 0) !important; */
}
+}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 1328-1328: expected } but instead the file ends
the file ends here
(parse)
There was a problem hiding this comment.
☝️ Address both the comments @gurramkarthiknetha
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
src/components/Common/ButtonV2.tsx
Outdated
| @@ -101,7 +101,7 @@ export const buttonStyles = ({ | |||
| shadow = !ghost, | |||
| }: ButtonStyleProps) => { | |||
| return classNames( | |||
| "inline-flex h-min cursor-pointer items-center justify-center gap-2 whitespace-pre font-medium outline-offset-1 transition-all duration-200 ease-in-out disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500", | |||
| "inline-flex h-min cursor-pointer items-center justify-center gap-2 whitespace-pre mt-2 font-medium outline-offset-1 transition-all duration-200 ease-in-out disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500", | |||
There was a problem hiding this comment.
ButtonV2 is widely used - make sure this doesn't cause any issues elsewhere. Instead of modifying it here, modify wherever the specific issue is occurring.
src/style/index.css
Outdated
| @media screen and (max-width:915px){ | ||
| .containerss{ | ||
| flex-wrap: wrap; | ||
| gap: 5px; | ||
| margin-bottom: 5px; | ||
| } | ||
| .bshadow{ | ||
| border:solid rgb(168, 168, 168); | ||
| /* box-shadow: 3px solid rgb(0, 0, 0) !important; */ | ||
| } | ||
| } |
There was a problem hiding this comment.
☝️ Address both the comments @gurramkarthiknetha
|
Also follow the PR format what we have (refer to other PRs). Edit the title to be readable as well. |
fixed the responsiveness and some file changes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/style/index.css (2)
1319-1323: Consider renaming the class for better maintainabilityThe class name
containerssappears to have a typo (extra 's'). Consider renaming it tocontaineror a more semantic name that describes its purpose.- .containerss{ + .container{ flex-wrap: wrap; gap: 5px; margin-bottom: 5px; }
1324-1326: Improve border specificationThe border property is missing width specification. Consider using a more explicit border declaration for better consistency.
.bshadow{ - border:solid rgb(168, 168, 168); + border: 1px solid rgb(168, 168, 168); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Common/ButtonV2.tsx(1 hunks)src/style/index.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Common/ButtonV2.tsx
🧰 Additional context used
🪛 Biome
src/style/index.css
[error] 1330-1330: expected } but instead the file ends
the file ends here
(parse)
🔇 Additional comments (1)
src/style/index.css (1)
1318-1330: LGTM! The media query implementation looks good
The responsive adjustments for smaller screens are well-structured and the use of rem units in .mtrequired is good for accessibility.
🧰 Tools
🪛 Biome
[error] 1330-1330: expected } but instead the file ends
the file ends here
(parse)
src/components/Common/ButtonV2.tsx
Outdated
| @@ -101,7 +101,7 @@ export const buttonStyles = ({ | |||
| shadow = !ghost, | |||
| }: ButtonStyleProps) => { | |||
| return classNames( | |||
| "inline-flex h-min cursor-pointer items-center justify-center gap-2 whitespace-pre font-medium outline-offset-1 transition-all duration-200 ease-in-out disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500", | |||
| "inline-flex h-min cursor-pointer items-center justify-center gap-2 whitespace-pre font-medium outline-offset-1 transition-all duration-200 ease-in-out disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500 mtrequired", | |||
There was a problem hiding this comment.
As I mentioned in the previous comment, let's not change anything in ButtonV2, but adjust where the issue occurs.
| @@ -36,7 +36,7 @@ export default function DropdownMenu({ | |||
| <Menu as="div" className="relative inline-block w-full text-left"> | |||
| <MenuButton | |||
| disabled={props.disabled} | |||
| className={`button-size-${size} button-${variant}-default button-shape-square flex w-full cursor-pointer items-center justify-center gap-2 font-medium outline-offset-1 transition-all duration-200 ease-in-out disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500 lg:justify-between ${props.className}`} | |||
| className={`button-size-${size} button-${variant}-default button-shape-square flex w-full cursor-pointer items-center justify-center gap-2 font-medium outline-offset-1 transition-all duration-200 ease-in-out disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500 mt-2 lg:justify-between ${props.className}`} | |||
There was a problem hiding this comment.
Make sure it isn't causing any issues elsewhere as Menu is used throughout.
src/components/Kanban/Board.tsx
Outdated
| @@ -58,7 +58,7 @@ export default function KanbanBoard<T extends { id: string }>( | |||
| </div> | |||
| <DragDropContext onDragEnd={props.onDragEnd}> | |||
| <div className="h-full overflow-scroll" ref={board}> | |||
| <div className="flex items-stretch px-0 pb-2"> | |||
| <div className="flex items-stretch px-0 pb-2 containerss"> | |||
There was a problem hiding this comment.
Tailwind allows for custom breakpoints for one-off cases, let's use that instead of creating a new class.
https://tailwindcss.com/docs/responsive-design#using-custom-breakpoints Check the very last section.
src/components/Kanban/Board.tsx
Outdated
| @@ -146,7 +146,7 @@ export function KanbanSection<T extends { id: string }>( | |||
| <div | |||
| ref={provided.innerRef} | |||
| className={ | |||
| "relative mr-2 w-[300px] shrink-0 rounded-xl bg-secondary-200" | |||
| "relative mr-2 w-[300px] shrink-0 rounded-xl bg-secondary-200 bshadow" | |||
There was a problem hiding this comment.
🤔
(1) Check the other examples of border in FE; use tailwind border instead of creating a custom class.
(2) Also, make sure to use names that make sense - why refer to this as shadow when it's a border?
Also this ☝️ @gurramkarthiknetha |
changed some required changes as required
fixed responsive ness
|
Now i kept from the same branch . |
src/components/Common/ButtonV2.tsx
Outdated
| @@ -101,7 +101,7 @@ export const buttonStyles = ({ | |||
| shadow = !ghost, | |||
| }: ButtonStyleProps) => { | |||
| return classNames( | |||
| "inline-flex h-min cursor-pointer items-center justify-center gap-2 whitespace-pre font-medium outline-offset-1 transition-all duration-200 ease-in-out disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500", | |||
| "inline-flex h-min cursor-pointer mt-2 items-center justify-center gap-2 whitespace-pre font-medium outline-offset-1 transition-all duration-200 ease-in-out disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500 mtrequired", | |||
There was a problem hiding this comment.
Since you have added mt-2 to the SlideOver page, let's get this (and the mtrequired) removed 👍 @gurramkarthiknetha
Other than that, it's fine.
package-lock.json
Outdated
| "integrity": "sha512-iRDPJKUPVEND7dHPO8rkbOnPpyDygcDFtWjpeWNCgy8WP2rXcxXL8TskReQl6OrB2G7+UJrags1q15Fudc7G6w==", | ||
| "version": "7.0.6", | ||
| "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-7.0.6.tgz", | ||
| "integrity": "sha512-uV2QOWP2nWzsy2aMp8aRibhi9dlzF5Hgh5SHaB9OiTGEyDTiJJyx0uy51QXdyWbtAHNua4XJzUKca3OzKUd3vA==", |
There was a problem hiding this comment.
Leave out package-lock from the commit please.
src/components/Kanban/Board.tsx
Outdated
| @@ -146,7 +146,7 @@ export function KanbanSection<T extends { id: string }>( | |||
| <div | |||
| ref={provided.innerRef} | |||
| className={ | |||
| "relative mr-2 w-[300px] shrink-0 rounded-xl bg-secondary-200" | |||
| "relative mr-2 w-[300px] shrink-0 rounded-xl bg-secondary-200 max-[915px]:border max-[915px]:border-solid max-[915px]:border-[rgb(168,168,168)]" | |||
There was a problem hiding this comment.
pick a color from the gray shade instead of using an arbitrary color. let's stick to the design system.
src/components/Kanban/Board.tsx
Outdated
| @@ -58,7 +58,7 @@ export default function KanbanBoard<T extends { id: string }>( | |||
| </div> | |||
| <DragDropContext onDragEnd={props.onDragEnd}> | |||
| <div className="h-full overflow-scroll" ref={board}> | |||
| <div className="flex items-stretch px-0 pb-2"> | |||
| <div className="flex items-stretch px-0 pb-2 max-[915px]:flex-wrap max-[915px]:gap-5 max-[915px]:mb-5"> | |||
There was a problem hiding this comment.
use standard width modifiers such as md: / lg: / xl: / 2xl:
|
@gurramkarthiknetha It's required to follow the original PR template and link the issue in it. Also keep a proper readable title that conveys what has changed in the PR. |
final change
gurramkarthiknetha
changed the title
|
now is this ok |
Proposed Changes
here i changed the hap between the red button and green oulined button
-
here i changed the responsive-ness wrap
@ohcnetwork/care-fe-code-reviewers
Merge Checklist