-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[WEB-2442] fix: Timeline Improvements and bug fixes #5922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
dd94c94
ac2723e
779e98d
d094c19
4e7db2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -98,11 +98,11 @@ export const GanttChartMainContent: React.FC<Props> = observer((props) => { | |||||||||||||||||||||||||||
| const onScroll = (e: React.UIEvent<HTMLDivElement, UIEvent>) => { | ||||||||||||||||||||||||||||
| const { clientWidth, scrollLeft, scrollWidth } = e.currentTarget; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const approxRangeLeft = scrollLeft >= clientWidth + 1000 ? 1000 : scrollLeft - clientWidth; | ||||||||||||||||||||||||||||
| const approxRangeLeft = scrollLeft; | ||||||||||||||||||||||||||||
| const approxRangeRight = scrollWidth - (scrollLeft + clientWidth); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if (approxRangeRight < 1000) updateCurrentViewRenderPayload("right", currentView); | ||||||||||||||||||||||||||||
| if (approxRangeLeft < 1000) updateCurrentViewRenderPayload("left", currentView); | ||||||||||||||||||||||||||||
| if (approxRangeRight < clientWidth) updateCurrentViewRenderPayload("right", currentView); | ||||||||||||||||||||||||||||
| if (approxRangeLeft < clientWidth) updateCurrentViewRenderPayload("left", currentView); | ||||||||||||||||||||||||||||
|
Comment on lines
+101
to
+105
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve scroll handling readability and maintainability. The scroll handling logic improvements are good - using Consider these improvements: - const approxRangeLeft = scrollLeft;
- const approxRangeRight = scrollWidth - (scrollLeft + clientWidth);
+ const distanceFromStart = scrollLeft;
+ const distanceFromEnd = scrollWidth - (scrollLeft + clientWidth);
+ const LOAD_THRESHOLD = clientWidth; // pixels before edge to trigger load
- if (approxRangeRight < clientWidth) updateCurrentViewRenderPayload("right", currentView);
- if (approxRangeLeft < clientWidth) updateCurrentViewRenderPayload("left", currentView);
+ if (distanceFromEnd < LOAD_THRESHOLD) updateCurrentViewRenderPayload("right", currentView);
+ if (distanceFromStart < LOAD_THRESHOLD) updateCurrentViewRenderPayload("left", currentView);This refactor:
📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const CHART_VIEW_COMPONENTS: { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,15 @@ import { useTimeLineChartStore } from "@/hooks/use-timeline-chart"; | |
| import { SIDEBAR_WIDTH } from "../constants"; | ||
| import { currentViewDataWithView } from "../data"; | ||
| import { ChartDataType, IBlockUpdateData, IBlockUpdateDependencyData, TGanttViews } from "../types"; | ||
| import { getNumberOfDaysBetweenTwoDates, IMonthBlock, IMonthView, IWeekBlock, timelineViewHelpers } from "../views"; | ||
| import { | ||
| getNumberOfDaysBetweenTwoDates, | ||
| IMonthBlock, | ||
| IMonthView, | ||
| IWeekBlock, | ||
| monthView, | ||
| quarterView, | ||
| weekView, | ||
| } from "../views"; | ||
|
|
||
| type ChartViewRootProps = { | ||
| border: boolean; | ||
|
|
@@ -35,6 +43,12 @@ type ChartViewRootProps = { | |
| showToday: boolean; | ||
| }; | ||
|
|
||
| const timelineViewHelpers = { | ||
| week: weekView, | ||
| month: monthView, | ||
| quarter: quarterView, | ||
| }; | ||
|
|
||
| export const ChartViewRoot: FC<ChartViewRootProps> = observer((props) => { | ||
| const { | ||
| border, | ||
|
|
@@ -62,8 +76,15 @@ export const ChartViewRoot: FC<ChartViewRootProps> = observer((props) => { | |
| const [itemsContainerWidth, setItemsContainerWidth] = useState(0); | ||
| const [fullScreenMode, setFullScreenMode] = useState(false); | ||
| // hooks | ||
| const { currentView, currentViewData, renderView, updateCurrentView, updateCurrentViewData, updateRenderView } = | ||
| useTimeLineChartStore(); | ||
| const { | ||
| currentView, | ||
| currentViewData, | ||
| renderView, | ||
| updateCurrentView, | ||
| updateCurrentViewData, | ||
| updateRenderView, | ||
| updateAllBlocksOnChartChangeWhileDragging, | ||
| } = useTimeLineChartStore(); | ||
|
|
||
| const updateCurrentViewRenderPayload = (side: null | "left" | "right", view: TGanttViews) => { | ||
| const selectedCurrentView: TGanttViews = view; | ||
|
|
@@ -89,6 +110,7 @@ export const ChartViewRoot: FC<ChartViewRootProps> = observer((props) => { | |
| updateCurrentView(selectedCurrentView); | ||
| updateRenderView(mergeRenderPayloads(currentRender.payload, renderView)); | ||
| updatingCurrentLeftScrollPosition(currentRender.scrollWidth); | ||
| updateAllBlocksOnChartChangeWhileDragging(currentRender.scrollWidth); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider updating blocks for both scroll directions. The Consider applying the same update for right scroll operations: } else if (side === "right") {
updateCurrentView(view);
updateRenderView(mergeRenderPayloads(renderView, currentRender.payload));
+ updateAllBlocksOnChartChangeWhileDragging(currentRender.scrollWidth);
setItemsContainerWidth(itemsContainerWidth + currentRender.scrollWidth);
|
||
| setItemsContainerWidth(itemsContainerWidth + currentRender.scrollWidth); | ||
| } else if (side === "right") { | ||
| updateCurrentView(view); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -66,13 +66,16 @@ export const useGanttResizable = ( | |||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| let width = initialPositionRef.current.width; | ||||||||||||||||||||||||||||||||||||||||||||||||
| let marginLeft = initialPositionRef.current.marginLeft; | ||||||||||||||||||||||||||||||||||||||||||||||||
| const blockRight = initialPositionRef.current.width + initialPositionRef.current.marginLeft; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| if (dragDirection === "left") { | ||||||||||||||||||||||||||||||||||||||||||||||||
| // calculate new marginLeft and update the initial marginLeft to the newly calculated one | ||||||||||||||||||||||||||||||||||||||||||||||||
| marginLeft = Math.round(mouseX / dayWidth) * dayWidth; | ||||||||||||||||||||||||||||||||||||||||||||||||
| // get Dimensions from dom's style | ||||||||||||||||||||||||||||||||||||||||||||||||
| const prevMarginLeft = parseFloat(resizableDiv.style.transform.slice(11, -3)); | ||||||||||||||||||||||||||||||||||||||||||||||||
| const prevWidth = parseFloat(resizableDiv.style.width.slice(0, -2)); | ||||||||||||||||||||||||||||||||||||||||||||||||
| // calculate new width | ||||||||||||||||||||||||||||||||||||||||||||||||
| width = blockRight - marginLeft; | ||||||||||||||||||||||||||||||||||||||||||||||||
| const marginDelta = prevMarginLeft - marginLeft; | ||||||||||||||||||||||||||||||||||||||||||||||||
| width = prevWidth + marginDelta; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+73
to
+78
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve robustness of dimension retrieval logic. The current implementation has potential fragility issues:
Consider this safer implementation: - const prevMarginLeft = parseFloat(resizableDiv.style.transform.slice(11, -3));
- const prevWidth = parseFloat(resizableDiv.style.width.slice(0, -2));
+ // Extract translateX value using regex
+ const transformMatch = resizableDiv.style.transform.match(/translateX\(([-\d.]+)px\)/);
+ const prevMarginLeft = transformMatch ? parseFloat(transformMatch[1]) : 0;
+
+ // Safely parse width
+ const prevWidth = resizableDiv.style.width ? parseFloat(resizableDiv.style.width) : 0;
+
+ // Validate parsed values
+ if (isNaN(prevMarginLeft) || isNaN(prevWidth)) {
+ console.warn('Invalid block dimensions detected');
+ return;
+ }📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||
| } else if (dragDirection === "right") { | ||||||||||||||||||||||||||||||||||||||||||||||||
| // calculate new width and update the initialMarginLeft using += | ||||||||||||||||||||||||||||||||||||||||||||||||
| width = Math.round(mouseX / dayWidth) * dayWidth - marginLeft; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ import uniqBy from "lodash/uniqBy"; | |
| // | ||
| import { months } from "../data"; | ||
| import { ChartDataType } from "../types"; | ||
| import { getNumberOfDaysInMonth } from "./helpers"; | ||
| import { getNumberOfDaysBetweenTwoDates, getNumberOfDaysInMonth } from "./helpers"; | ||
| import { getWeeksBetweenTwoDates, IWeekBlock } from "./week-view"; | ||
|
|
||
| export interface IMonthBlock { | ||
|
|
@@ -38,6 +38,9 @@ const generateMonthChart = (monthPayload: ChartDataType, side: null | "left" | " | |
| let minusDate: Date = new Date(); | ||
| let plusDate: Date = new Date(); | ||
|
|
||
| let startDate = new Date(); | ||
| let endDate = new Date(); | ||
|
|
||
| // if side is null generate months on both side of current date | ||
| if (side === null) { | ||
| const currentDate = renderState.data.currentDate; | ||
|
|
@@ -47,12 +50,14 @@ const generateMonthChart = (monthPayload: ChartDataType, side: null | "left" | " | |
|
|
||
| if (minusDate && plusDate) filteredDates = getMonthsViewBetweenTwoDates(minusDate, plusDate); | ||
|
|
||
| startDate = filteredDates.weeks[0]?.startDate; | ||
| endDate = filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate; | ||
| renderState = { | ||
| ...renderState, | ||
| data: { | ||
| ...renderState.data, | ||
| startDate: filteredDates.weeks[0]?.startDate, | ||
| endDate: filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate, | ||
| startDate, | ||
| endDate, | ||
|
Comment on lines
+53
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add null safety checks for weeks array access. The code assumes Apply this diff to add null safety: - startDate = filteredDates.weeks[0]?.startDate;
- endDate = filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate;
+ startDate = filteredDates.weeks[0]?.startDate ?? minusDate;
+ endDate = filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate ?? plusDate;
|
||
| }, | ||
| }; | ||
| } | ||
|
|
@@ -65,9 +70,11 @@ const generateMonthChart = (monthPayload: ChartDataType, side: null | "left" | " | |
|
|
||
| if (minusDate && plusDate) filteredDates = getMonthsViewBetweenTwoDates(minusDate, plusDate); | ||
|
|
||
| startDate = filteredDates.weeks[0]?.startDate; | ||
| endDate = new Date(currentDate.getFullYear(), currentDate.getMonth(), currentDate.getDate() - 1); | ||
| renderState = { | ||
| ...renderState, | ||
| data: { ...renderState.data, startDate: filteredDates.weeks[0].startDate }, | ||
| data: { ...renderState.data, startDate }, | ||
| }; | ||
| } | ||
| // When side is right, generate more months on the right side of the end date | ||
|
|
@@ -79,13 +86,16 @@ const generateMonthChart = (monthPayload: ChartDataType, side: null | "left" | " | |
|
|
||
| if (minusDate && plusDate) filteredDates = getMonthsViewBetweenTwoDates(minusDate, plusDate); | ||
|
|
||
| startDate = new Date(currentDate.getFullYear(), currentDate.getMonth(), currentDate.getDate() + 1); | ||
| endDate = filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate; | ||
| renderState = { | ||
| ...renderState, | ||
| data: { ...renderState.data, endDate: filteredDates.weeks[filteredDates.weeks.length - 1]?.endDate }, | ||
| }; | ||
| } | ||
|
|
||
| const scrollWidth = filteredDates.weeks.length * monthPayload.data.dayWidth * 7; | ||
| const days = Math.abs(getNumberOfDaysBetweenTwoDates(startDate, endDate)) + 1; | ||
| const scrollWidth = days * monthPayload.data.dayWidth; | ||
|
|
||
| return { state: renderState, payload: filteredDates, scrollWidth: scrollWidth }; | ||
| }; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding validation and optimizing performance.
The new method implementation has several areas for improvement:
addedWidthparameter.Here's a suggested implementation with these improvements:
updateAllBlocksOnChartChangeWhileDragging = action((addedWidth: number) => { + if (typeof addedWidth !== 'number' || isNaN(addedWidth)) { + console.warn('Invalid width adjustment provided'); + return; + } if (!this.blockIds || !this.isDragging) return; runInAction(() => { - this.blockIds?.forEach((blockId) => { + // Optional: Filter visible blocks first + const visibleBlocks = this.blockIds?.filter((blockId) => { + const block = this.blocksMap[blockId]; + return block?.position?.marginLeft !== undefined; + }); + + visibleBlocks?.forEach((blockId) => { const currBlock = this.blocksMap[blockId]; if (!currBlock || !currBlock.position) return; + try { currBlock.position.marginLeft += addedWidth; + } catch (error) { + console.error(`Failed to update position for block ${blockId}:`, error); + } }); }); });