fix: Adjust containers flexibility and inconsistency #10553
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces modifications to the layout and structure of the Changes
Sequence Diagram(s)(No sequence diagrams generated as the changes only affect UI styling and layout adjustments without modifying control flow.) Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. |
✅ 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: 0
🧹 Nitpick comments (2)
src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx (2)
434-468: Simplify the time fields container layout.The current implementation has redundant height constraints and unnecessary margins that could cause layout issues.
Consider this simplified layout:
-<div className="flex items-center gap-0 sm:gap-4 col-span-2 md:col-span-1 h-28"> - <div className="flex items-center justify-center mt-16"> +<div className="flex items-center gap-0 sm:gap-4 col-span-2 md:col-span-1"> + <div className="flex items-center justify-center"> <FormField control={form.control} name={`availabilities.${index}.start_time`} render={({ field }) => ( - <FormItem className="flex-1 mb-12 h-28"> + <FormItem className="flex-1"> <FormLabel required> {t("start_time")} </FormLabel> <FormControl> <Input type="time" {...field} /> </FormControl> <FormMessage /> </FormItem> )} /> - <ArrowRightIcon className="size-4 mb-20" /> + <ArrowRightIcon className="size-4 mx-2" /> <FormField control={form.control} name={`availabilities.${index}.end_time`} render={({ field }) => ( - <FormItem className="flex-1 mb-12 h-28"> + <FormItem className="flex-1"> <FormLabel required>{t("end_time")}</FormLabel> <FormControl> <Input type="time" {...field} /> </FormControl> <FormMessage /> </FormItem> )} /> </div> </div>
473-529: Standardize the slot configuration fields layout.The container has redundant height constraints and inconsistent spacing.
Consider this simplified layout:
-<div className="flex items-center gap-2 sm:gap-4 col-span-2 md:col-span-1 h-28 pt-4"> +<div className="flex items-center gap-2 sm:gap-4 col-span-2 md:col-span-1"> <FormField control={form.control} name={`availabilities.${index}.slot_size_in_minutes`} render={({ field }) => ( - <FormItem className="flex-1 mt-4 mb-8 h-24"> + <FormItem className="flex-1"> <FormLabel required className="whitespace-nowrap" > {t("schedule_slot_size_label")} </FormLabel> <FormControl> <Input type="number" min={0} placeholder="e.g. 10" {...field} value={field.value ?? ""} onChange={(e) => field.onChange(e.target.valueAsNumber) } /> </FormControl> <FormMessage /> </FormItem> )} /> <FormField control={form.control} name={`availabilities.${index}.tokens_per_slot`} render={({ field }) => ( - <FormItem className="flex-1 mt-4 mb-8 h-24"> + <FormItem className="flex-1"> <FormLabel required className="whitespace-nowrap" > {t("patients_per_slot")} </FormLabel> <FormControl> <Input type="number" min={0} placeholder="e.g. 1" {...field} value={field.value ?? ""} onChange={(e) => field.onChange(e.target.valueAsNumber) } /> </FormControl> <FormMessage /> </FormItem> )} /> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx (2)
537-555: Clean implementation of the remarks field container.The container follows a clean layout without unnecessary height constraints or margins.
257-606: Well-structured form with proper validation and responsive layout.The form implementation demonstrates good practices:
- Proper grid layout for field organization
- Responsive design with mobile-first approach
- Comprehensive form validation
- Clear error handling
src/pages/Scheduling/components/CreateScheduleTemplateSheet.tsx
Outdated
Show resolved
Hide resolved
| control={form.control} | ||
| name={`availabilities.${index}.end_time`} | ||
| render={({ field }) => ( | ||
| <FormItem className="flex-1 mb-12 h-28"> |
There was a problem hiding this comment.
See if you can solve this without specifying a particular height for the FormItem components.
There was a problem hiding this comment.
I think we have to set the height in both FormItems because we are showing message in one container so it will get adjusted automatically if we will not set the height. Is there any issue with this approach?🤔
There was a problem hiding this comment.
Yes, we don't want to randomly set height like that 😅
https://css-tricks.com/snippets/css/a-guide-to-flexbox/
You should be able to solve this without setting height.
There was a problem hiding this comment.
Ok sure. I will do it with different approach.👍
Tanuj1718
force-pushed
the
fix-containers
branch
from
69630df to
080f048
Compare
|
mark it for ready for review once changes have been made |
Proposed Changes
Before Update:
Screen.Recording.2025-02-10.at.10.39.05.AM.mov
After Update:
Screen.Recording.2025-02-11.at.1.49.21.PM.mov
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Style
Refactor