Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses slot selection issues on mobile devices by adjusting the longPressThreshold parameter and modifying CSS styling to improve clickable areas in the calendar component.
Changes:
- Added
longPressThresholdprop set to 50ms to improve mobile touch responsiveness - Introduced a
handleSelectSlotwrapper function to handle slot selection events - Modified CSS styling to increase clickable areas in month view calendar cells
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| booking-creation-time-slot-selector.tsx | Added longPressThreshold={50} prop to BookingCalendar component |
| booking-calendar.tsx | Extracted longPressThreshold and onSelectSlot props, added wrapper function for slot selection |
| booking-calendar.module.scss | Adjusted padding and minimum height for calendar date cells to improve clickability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| :global(.rbc-month-view .rbc-date-cell) { | ||
| padding: 0; | ||
| min-height: 50px; /* Adjust height based on your design */ | ||
| padding-bottom: 10px; /* Ensures space below existing events is clickable */ |
There was a problem hiding this comment.
Removing padding: 0 and replacing it with padding-bottom: 10px will also add default padding to other sides of the element. If the intent was to only have bottom padding, consider using padding: 0 0 10px 0 to be more explicit and avoid unintended padding on other sides.
| padding-bottom: 10px; /* Ensures space below existing events is clickable */ | |
| padding: 0 0 10px 0; /* Ensures space below existing events is clickable */ |
| const { onRepeatSlot, onSelectSlot, longPressThreshold, ...calendarProps } = | ||
| props; | ||
|
|
||
| const handleSelectSlot = (slotInfo: any) => { |
There was a problem hiding this comment.
The parameter type should be explicitly typed instead of using any. Based on the react-big-calendar library, this should be typed as SlotInfo which includes properties like start, end, slots, and action.
| onSelectSlot={handleSelectSlot} | ||
| {...calendarProps} |
There was a problem hiding this comment.
The handleSelectSlot wrapper function doesn't add any meaningful logic - it only conditionally calls onSelectSlot if it exists. However, spreading calendarProps after explicitly setting onSelectSlot on line 93 means that if onSelectSlot exists in calendarProps, it will override this wrapper. This could bypass the wrapper entirely and may cause unexpected behavior. Consider either removing the wrapper if it's unnecessary, or ensure onSelectSlot is excluded from calendarProps when spreading.
| height: 100%; | ||
| padding: 0.25rem 0.5rem; | ||
| text-align: left; | ||
| padding-bottom: 150px; |
There was a problem hiding this comment.
The magic number 150px for padding-bottom is unusually large and lacks explanation. This seems excessive for ensuring clickable space and could cause significant layout issues. Please either add a comment explaining why 150px is needed, or verify this value is correct. If the goal is to make space clickable, a more reasonable value like 10-20px would typically suffice.
| padding-bottom: 150px; | |
| padding-bottom: 10px; /* Provide extra clickable space without distorting layout */ |
| onDrillDown={onDrillDown} | ||
| onRepeatSlot={onRepeatSlot} | ||
| selectable | ||
| longPressThreshold={50} |
There was a problem hiding this comment.
The magic number 50 for longPressThreshold is duplicated between this file (line 226) and the default value in booking-calendar.tsx (line 67). Consider extracting this to a named constant to ensure consistency and make it easier to adjust this value across the codebase if needed.
No description provided.