#3845 mouse placement bug#3966
#3845 mouse placement bug#3966Santiordon wants to merge 6 commits intofeature/gantt-chart-improvementsfrom
Conversation
|
oh dear lord |
There was a problem hiding this comment.
Pull request overview
This pull request addresses a mouse placement bug (#3928) in the Gantt chart where editing the length or start date of a Gantt element was off by one position from where the user released the mouse.
Changes:
- Refactored mouse event handling to calculate width based on absolute position from the left edge rather than tracking deltas
- Changed rounding behavior from Math.round to Math.ceil for week calculations
- Added correctWidth state to maintain proper display width during resizing
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const getDistanceFromLeft = (clientX: number) => { | ||
| const rect = boxRef.current!.getBoundingClientRect(); | ||
| return clientX - rect.left; | ||
| }; |
There was a problem hiding this comment.
This function uses a non-null assertion operator on boxRef.current without checking if it's null. If handleMouseMove is called before handleMouseDown sets boxRef.current, or if the closest() call returns null, this will throw a runtime error. Consider adding a null check to prevent potential crashes.
| const getCorrectWidth = useCallback((rawWidth: number) => { | ||
| const newEventLengthInDays = roundToMultipleOf7(rawWidth / widthPerDay); | ||
| const displayWeeks = newEventLengthInDays / 7 + 1; | ||
| return displayWeeks * 40 + (displayWeeks - 1) * 10; |
There was a problem hiding this comment.
The magic number 40 appears to be changed from 38, but this is inconsistent with GANTT_CHART_CELL_SIZE which is defined as '2.375rem' (approximately 38px at 16px base font size). This hardcoded value should either use the constant or have a clear explanation for the 2px discrepancy. Similarly, the gap value of 10 doesn't match GANTT_CHART_GAP_SIZE ('0.75rem' = 12px). Consider using the constants or deriving pixel values from them to maintain consistency and make the calculation more maintainable.
| const getCorrectWidth = useCallback((rawWidth: number) => { | ||
| const newEventLengthInDays = roundToMultipleOf7(rawWidth / widthPerDay); | ||
| const displayWeeks = newEventLengthInDays / 7 + 1; | ||
| return displayWeeks * 40 + (displayWeeks - 1) * 10; | ||
| }, []); |
There was a problem hiding this comment.
The useCallback hook is missing widthPerDay in its dependency array. Since getCorrectWidth references widthPerDay (line 73), it should be included in the dependencies. However, since widthPerDay is a constant defined in the component, consider moving it outside the component as a module-level constant to avoid this issue and improve performance.
| const roundToMultipleOf7 = (num: number) => { | ||
| return Math.round(num / 7) * 7; | ||
| return Math.ceil(num / 7) * 7; | ||
| }; |
There was a problem hiding this comment.
The roundToMultipleOf7 function is defined within the component body and referenced by getCorrectWidth. This causes the function to be redefined on every render. Consider moving roundToMultipleOf7 outside the component as a module-level utility function to improve performance and avoid unnecessary re-creations.
| const handleMouseDown = (e: MouseEvent<HTMLElement>) => { | ||
| setIsResizing(true); | ||
| setStartX(e.clientX); | ||
| boxRef.current = (e.currentTarget as HTMLElement).closest('[data-gantt-bar]') as HTMLDivElement; |
There was a problem hiding this comment.
The closest() method can return null if no matching ancestor is found. This code uses type assertion to assume it's always an HTMLDivElement, which could lead to runtime errors. Consider adding a null check or using optional chaining to handle cases where the data-gantt-bar attribute might not be found in the element's ancestors.
| // used to make sure that any changes to the start and end dates are made in multiples of 7 | ||
| const roundToMultipleOf7 = (num: number) => { | ||
| return Math.round(num / 7) * 7; | ||
| return Math.ceil(num / 7) * 7; |
There was a problem hiding this comment.
Changing from Math.round to Math.ceil is a significant behavioral change. With Math.round, a value of 10.4 days would round down to 7 days, but with Math.ceil it will round up to 14 days. This means users will always get a larger-than-expected duration when resizing tasks. While this may fix the "one off bug", it could lead to unexpected behavior where tasks become longer than the user intended. Consider whether this is the desired behavior or if a more sophisticated rounding strategy is needed.
| return Math.ceil(num / 7) * 7; | |
| const weeks = Math.max(1, Math.round(num / 7)); | |
| return weeks * 7; |
chpy04
left a comment
chpy04
left a comment
There was a problem hiding this comment.
I agree with most of copilots comments on this. Also, this might have not been clear in the bug, but the two bugs with moving things on the gantt chart right now are:
- when I initially go to drag the end date of something, it initially shortens a lot and then I can drag my mouse around to change the date (this appears to be fixed)
- when moving the start date, it had the off by one bug (which also appears to be fixed)
however we now seem to have the inverse problem when dragging the end date, where it tends to go one slot further than I would expect it to. I think this might be from the math.round -> math.ceil, but im not sure which functionality that is actually refering to
2 participants
Changes
Changed GanttTaskBarEditView.tsx to reflect an accurate date mapping instead of being skewed one off.
Notes
None
Test Cases
N/A
Screenshots
To Do
N/A
Checklist
yarn.lockchanges (unless dependencies have changed)Closes #3928