Skip to content
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

[WEB-2442] feat: Minor Timeline view Enhancements #5987

Merged
merged 8 commits into from
Nov 13, 2024

Conversation

rahulramesha
Copy link
Collaborator

@rahulramesha rahulramesha commented Nov 11, 2024

This PR contains minor enhancements to Timeline layouts along with few fixes, Couple of the enhancements are,

  1. Sticky block name, to enable readability of the block regardless of scroll position
Screen.Recording.2024-11-12.at.5.07.10.PM.mov
  1. Enable to view blocks with just Start or End date without the need of having them both
Screen.Recording.2024-11-12.at.5.08.31.PM.mov

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced new methods for calculating block positions based on dates in the timeline.
    • Enhanced drag-and-drop functionality with specific conditions for valid drop targets.
    • Added tooltips for better information display in Gantt chart components.
  • Improvements

    • Simplified visibility logic for blocks in the Gantt chart.
    • Improved scroll handling and layout adjustments in the Gantt chart.
    • Enhanced error handling in resizing logic for Gantt blocks.
  • Bug Fixes

    • Corrected error handling in block resizing to ensure proper feedback.
  • Style

    • Updated styling approach in several components to improve consistency and responsiveness.

Copy link
Contributor

coderabbitai bot commented Nov 11, 2024

Walkthrough

The changes in this pull request primarily enhance the functionality and logic of the Gantt chart and timeline components. Key modifications include the addition of a new method in the BaseTimeLineStore for date-to-position calculations, updates to visibility logic in various Gantt chart components, and improvements in styling and event handling. The introduction of constants and utility functions also refines the overall structure, ensuring better management of block rendering and interaction across the Gantt chart.

Changes

File Path Change Summary
web/ce/store/timeline/base-timeline.store.ts - Added method getPositionFromDateOnGantt in BaseTimeLineStore and updated IBaseTimelineStore interface.
- Modified updateBlocks method to change visibility conditions for block updates.
web/core/components/gantt-chart/blocks/block-row.tsx - Updated visibility logic for blocks to check for either block.start_date or block.target_date.
- Adjusted rendering conditions for the scroll button based on visibility.
web/core/components/gantt-chart/blocks/block.tsx - Introduced isBlockVisibleOnChart and isBlockComplete variables for visibility logic.
- Adjusted rendering logic to improve clarity and restricted block movement based on completion status.
web/core/components/gantt-chart/chart/main-content.tsx - Imported DEFAULT_BLOCK_WIDTH and modified scroll handling logic.
- Adjusted handleScrollToBlock for better scroll position calculation.
web/core/components/gantt-chart/chart/views/month.tsx - Removed inline minHeight style, replaced with min-h-full class for height management.
web/core/components/gantt-chart/chart/views/quarter.tsx - Similar change as in month.tsx, replaced inline minHeight with min-h-full.
web/core/components/gantt-chart/chart/views/week.tsx - Similar change as in month.tsx and quarter.tsx, replaced inline minHeight with min-h-full.
web/core/components/gantt-chart/constants.ts - Added constant DEFAULT_BLOCK_WIDTH with a value of 60.
web/core/components/gantt-chart/helpers/blockResizables/use-gantt-resizable.ts - Modified handleMouseMove to use marginLeft for resizing calculations.
- Corrected error handling in handleMouseUp function.
web/core/components/gantt-chart/sidebar/gantt-dnd-HOC.tsx - Enhanced drag-and-drop functionality by adding dragInstanceId checks in canDrop and onDrop methods.
web/core/components/gantt-chart/sidebar/issues/block.tsx - Renamed variable isBlockVisibleOnChart to isBlockComplete, affecting duration calculations.
web/core/components/gantt-chart/sidebar/modules/block.tsx - Similar change as in issues/block.tsx, renamed variable isBlockVisibleOnChart to isBlockComplete.
web/core/components/gantt-chart/views/helpers.ts - Added getPositionFromDate function for date-to-position calculations.
- Refined logic in getItemPositionWidth for handling date calculations.
web/core/components/issues/issue-layouts/gantt/blocks.tsx - Updated rendering logic in IssueGanttBlock to use a Tooltip component and adjusted event handling in IssueGanttSidebarBlock.
web/core/components/issues/issue-layouts/utils.tsx - Added getBlockViewDetails function for generating tooltip messages based on block visibility and completeness.
web/core/components/modules/gantt-chart/blocks.tsx - Enhanced tooltip functionality in ModuleGanttBlock by using getBlockViewDetails for dynamic styling and content.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant GanttChart
    participant BaseTimeLineStore
    participant Block

    User->>GanttChart: Interacts with Gantt chart
    GanttChart->>BaseTimeLineStore: Request position from date
    BaseTimeLineStore->>BaseTimeLineStore: Calculate position using getPositionFromDate
    BaseTimeLineStore-->>GanttChart: Return calculated position
    GanttChart->>Block: Render block at position
Loading

Possibly related PRs

Suggested labels

🌟enhancement, 🐛bug, 🌐frontend

Suggested reviewers

  • SatishGandham

Poem

🐰 In the garden of code, where the timelines grow,
New paths are drawn, and the blocks start to flow.
With dates and widths, we dance and we play,
Enhancing the chart in a whimsical way!
So hop along, friends, let’s celebrate bright,
For our Gantt chart shines with new features in sight! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@rahulramesha rahulramesha changed the title feat: Minor Timeline view Enhancements [WEB-2442] feat: Minor Timeline view Enhancements Nov 12, 2024
@rahulramesha rahulramesha marked this pull request as ready for review November 12, 2024 11:43
@rahulramesha rahulramesha added 🌐frontend 🐛bug Something isn't working 🌟enhancement New feature or request labels Nov 12, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (24)
web/core/components/gantt-chart/constants.ts (1)

9-10: Consider adding documentation and flexibility to the block width constant.

While the constant follows the established naming pattern, consider these improvements:

  1. Add a comment explaining its purpose and unit (pixels)
  2. Consider making it configurable based on viewport width or content requirements, especially for the new sticky block name feature
+// Default width (in pixels) for timeline blocks. Used for calculating block dimensions and scroll positions.
 export const DEFAULT_BLOCK_WIDTH = 60;
web/core/components/gantt-chart/sidebar/modules/block.tsx (1)

Line range hint 41-47: Consider enhancing date visibility for better UX

Since the PR aims to support blocks with either Start or End date, consider enhancing the UI to show which date is available when only one is present. This would improve user understanding of partially configured blocks.

Consider updating the duration display:

           <div className="flex-grow truncate">
             <ModuleGanttSidebarBlock moduleId={block.data.id} />
           </div>
-          {duration !== undefined && (
+          {(block.start_date || block.target_date) && (
             <div className="flex-shrink-0 text-sm text-custom-text-200">
-              {duration} day{duration > 1 ? "s" : ""}
+              {duration !== undefined ? (
+                `${duration} day${duration > 1 ? "s" : ""}`
+              ) : (
+                `${block.start_date ? "Started" : "Due"}`
+              )}
             </div>
           )}
web/core/components/gantt-chart/sidebar/issues/block.tsx (1)

Line range hint 89-95: Consider these enhancements for better maintainability

The duration display logic could be improved for better maintainability and accessibility.

Consider these improvements:

+const getDurationText = (days: number) => `${days} day${days > 1 ? 's' : ''}`;

 {duration && (
   <div className="flex-shrink-0 text-sm text-custom-text-200">
-    <span>
-      {duration} day{duration > 1 ? "s" : ""}
-    </span>
+    <span aria-label={`Duration: ${getDurationText(duration)}`}>
+      {getDurationText(duration)}
+    </span>
   </div>
 )}
web/core/components/gantt-chart/blocks/block.tsx (2)

52-53: Update comment to match the actual visibility logic.

The current comment suggests checking for both dates, but the code actually checks for either date.

-  // hide the block if it doesn't have start and target dates and showAllBlocks is false
+  // hide the block if it doesn't have either start or target date and showAllBlocks is false

67-67: Consider using transform for better performance.

Using marginLeft triggers layout recalculation. For better performance, especially during animations, consider using transform: translateX() which leverages GPU acceleration.

-        marginLeft: `${block.position?.marginLeft}px`,
+        transform: `translateX(${block.position?.marginLeft}px)`,
web/core/components/gantt-chart/chart/views/week.tsx (2)

Line range hint 19-91: Consider breaking down the component for better maintainability.

The component has multiple nested sections handling different concerns. Consider extracting these into separate components:

  • WeekHeader (lines 25-54)
  • WeekDayColumns (lines 71-89)

This would improve readability and make the code more maintainable.

Example structure:

interface WeekHeaderProps {
  block: IWeekBlock;
  dayWidth: number;
  rootIndex: number;
}

const WeekHeader: FC<WeekHeaderProps> = ({ block, dayWidth, rootIndex }) => {
  // Header implementation
};

interface WeekDayColumnsProps {
  block: IWeekBlock;
  dayWidth: number;
  rootIndex: number;
}

const WeekDayColumns: FC<WeekDayColumnsProps> = ({ block, dayWidth, rootIndex }) => {
  // Day columns implementation
};

Line range hint 31-33: Consider extracting magic numbers to constants.

The hardcoded values in styles could be moved to constants for better maintainability:

  • h-7 for month title height
  • h-5 for days subtitle height
const MONTH_TITLE_HEIGHT = 'h-7';
const DAYS_SUBTITLE_HEIGHT = 'h-5';

Also applies to: 45-47

web/core/components/gantt-chart/sidebar/gantt-dnd-HOC.tsx (2)

37-37: Extract "GANTT_REORDER" into a constant.

Consider defining this identifier as a named constant at the module level for better maintainability and reusability.

+const GANTT_REORDER_IDENTIFIER = "GANTT_REORDER";
+
 // ...inside component
-getInitialData: () => ({ id, dragInstanceId: "GANTT_REORDER" }),
+getInitialData: () => ({ id, dragInstanceId: GANTT_REORDER_IDENTIFIER }),

47-47: LGTM! Consider using the suggested constant.

The additional check for dragInstanceId is a good safety measure to ensure only Gantt reordering operations are processed. If you implement the suggested constant from the previous comment, update this line to use it as well.

-canDrop: ({ source }) => source?.data?.id !== id && source?.data?.dragInstanceId === "GANTT_REORDER",
+canDrop: ({ source }) => source?.data?.id !== id && source?.data?.dragInstanceId === GANTT_REORDER_IDENTIFIER,
web/core/components/gantt-chart/views/helpers.ts (2)

104-109: Consider improving width calculation robustness.

The width calculation could be enhanced in two ways:

  1. Add validation to ensure target_date is not before start_date
  2. Add more descriptive comments explaining the width calculation logic

Consider this improvement:

 if (itemStartDate && itemTargetDate) {
-    // get width of block
+    // Calculate block width: add 1 to include both start and end dates
+    if (itemTargetDate < itemStartDate) {
+      console.warn('Target date is before start date');
+    }
     const widthTimeDifference: number = itemStartDate.getTime() - itemTargetDate.getTime();
     const widthDaysDifference: number = Math.abs(Math.floor(widthTimeDifference / (1000 * 60 * 60 * 24)));
     scrollWidth = (widthDaysDifference + 1) * chartData.data.dayWidth;
 }

114-131: Consider enhancing function documentation.

While the function is well-implemented, its documentation could be improved:

  1. Add JSDoc explaining the purpose and return value
  2. Document the offsetWidth parameter's purpose and valid values

Add this documentation:

+/**
+ * Calculates the pixel position on the Gantt chart for a given date
+ * @param chartData - The chart configuration and dimensions
+ * @param date - The date to calculate position for
+ * @param offsetWidth - Pixel offset to adjust the final position (useful for positioning block endings)
+ * @returns The pixel position on the chart, or 0 if dates are invalid
+ */
 export const getPositionFromDate = (chartData: ChartDataType, date: string | Date, offsetWidth: number) => {
web/core/components/gantt-chart/blocks/block-row.tsx (4)

71-71: Consider renaming target_date to end_date for consistency.

While the logic change correctly implements the requirement to show blocks with either a start or end date, the variable naming (target_date) doesn't match the terminology used in the PR objectives ("End date"). This inconsistency might cause confusion for future maintainers.


Line range hint 42-63: Consider adding block properties to effect dependencies.

The intersection observer effect only depends on block but uses block.position, block.id, etc. inside. If these properties change but the block reference remains the same, the effect won't re-run. This could lead to stale calculations.

Consider updating the effect dependencies:

    return () => {
      observer.unobserve(timelineBlock);
    };
-  }, [block]);
+  }, [block?.id, block?.position?.marginLeft, block?.position?.width]);

Line range hint 66-66: Fix inconsistent visibility check in early return.

The early return condition (!showAllBlocks && !(block.start_date && block.target_date)) requires both dates when showAllBlocks is false, which contradicts the new visibility logic that allows either date.

Update the condition to match the new logic:

-  if (!block || !block.data || (!showAllBlocks && !(block.start_date && block.target_date))) return null;
+  if (!block || !block.data || (!showAllBlocks && !(block.start_date || block.target_date))) return null;

Line range hint 99-103: Consider using z-index constants.

The scroll button uses a hardcoded z-index value (z-[5]). This could lead to z-index wars if other components need to be layered relative to this button.

Consider defining z-index values as constants alongside other UI constants:

// constants.ts
export const Z_INDEXES = {
  SCROLL_BUTTON: 5,
  // ... other z-index values
} as const;

Then use it in the component:

-                className="sticky z-[5] grid h-8 w-8 translate-y-1.5 cursor-pointer place-items-center rounded border border-custom-border-300 bg-custom-background-80 text-custom-text-200 hover:text-custom-text-100"
+                className={`sticky z-[${Z_INDEXES.SCROLL_BUTTON}] grid h-8 w-8 translate-y-1.5 cursor-pointer place-items-center rounded border border-custom-border-300 bg-custom-background-80 text-custom-text-200 hover:text-custom-text-100`}
web/core/components/issues/issue-layouts/gantt/blocks.tsx (1)

64-65: Consider preventing event propagation for consistency

The click handler should prevent event propagation like its sidebar counterpart for consistent behavior.

-        onClick={handleIssuePeekOverview}
+        onClick={(e) => {
+          e.stopPropagation();
+          e.preventDefault();
+          handleIssuePeekOverview();
+        }}
web/core/components/gantt-chart/helpers/blockResizables/use-gantt-resizable.ts (3)

Line range hint 74-91: Refactor DOM manipulation for better React practices and reliability.

The current implementation has several potential issues:

  1. Direct DOM style manipulation bypasses React's virtual DOM
  2. Parsing style strings with slice(0, -2) is fragile
  3. Reading styles from DOM during mousemove events may impact performance

Consider refactoring to use React state and refs:

-const prevMarginLeft = parseFloat(resizableDiv.style.marginLeft.slice(0, -2));
-const prevWidth = parseFloat(resizableDiv.style.width.slice(0, -2));
+const prevMarginLeft = resizableRef.current?.offsetLeft ?? 0;
+const prevWidth = resizableRef.current?.offsetWidth ?? 0;

 // calculate new width
 const marginDelta = prevMarginLeft - marginLeft;
 width = prevWidth + marginDelta;
 
-resizableDiv.style.width = `${width}px`;
-resizableDiv.style.marginLeft = `${marginLeft}px`;
+const [dimensions, setDimensions] = useState({ width: 0, marginLeft: 0 });
+setDimensions({ width, marginLeft });

Then use these values in your component's style prop:

style={{ width: dimensions.width, marginLeft: dimensions.marginLeft }}

Line range hint 112-116: Fix error handling implementation.

The current error handling has several issues:

  1. setToast is not properly invoked
  2. The caught error is not being used to provide meaningful feedback
  3. Generic error handling might hide important issues

Apply this fix:

 try {
   const blockUpdates = getUpdatedPositionAfterDrag(block.id, dragDirection !== "move");
   updateBlockDates && updateBlockDates(blockUpdates);
 } catch (e) {
-  setToast;
+  setToast({
+    title: "Error updating block dates",
+    message: e instanceof Error ? e.message : "An unexpected error occurred",
+    type: "error",
+  });
+  console.error("Failed to update block dates:", e);
 }

Line range hint 15-124: Consider edge cases and performance optimizations.

The current implementation might benefit from additional safeguards and optimizations:

  1. Mouse events during drag operations could be throttled for better performance
  2. Event listeners should be cleaned up on component unmount
  3. Race conditions between mouse and scroll events should be handled

Consider these improvements:

  1. Add cleanup in a useEffect:
useEffect(() => {
  return () => {
    // Cleanup any active listeners on unmount
    if (ganttContainerRef.current) {
      ganttContainerRef.current.removeEventListener('scroll', handleOnScroll);
    }
    document.removeEventListener('mousemove', handleMouseMove);
    document.removeEventListener('mouseup', handleMouseUp);
  };
}, []);
  1. Throttle the mousemove handler:
import { throttle } from 'lodash';

const throttledMouseMove = throttle(handleMouseMove, 16); // ~60fps
  1. Add a mounted ref to prevent updates after unmount:
const isMounted = useRef(true);
useEffect(() => {
  return () => {
    isMounted.current = false;
  };
}, []);
web/core/components/gantt-chart/chart/main-content.tsx (2)

123-124: Document the magic number in scroll position calculation

The scroll position logic has been improved to handle blocks with either start or end dates. However, there's a magic number (-4) in the calculation that should be documented or extracted into a named constant for better maintainability.

- scrollContainer.scrollLeft = updatedPosition.marginLeft - 4 - (scrollToEndDate ? DEFAULT_BLOCK_WIDTH : 0);
+ // Define constant at the top of the file
+ const SCROLL_POSITION_OFFSET = 4; // Explain why this offset is needed
+ scrollContainer.scrollLeft = updatedPosition.marginLeft - SCROLL_POSITION_OFFSET - (scrollToEndDate ? DEFAULT_BLOCK_WIDTH : 0);

Also applies to: 138-139


199-199: Consider adding ARIA attributes for better accessibility

While the padding adjustment improves visual consistency, consider adding ARIA attributes to help screen readers understand the layout structure better.

<div
  className="relative h-full"
  style={{
    width: `${itemsContainerWidth}px`,
    transform: `translateY(${HEADER_HEIGHT}px)`,
    paddingBottom: `${HEADER_HEIGHT}px`,
  }}
+ role="region"
+ aria-label="Gantt chart timeline content"
>
web/ce/store/timeline/base-timeline.store.ts (2)

235-242: Consider adding input validation

While the implementation is clean, consider adding validation for:

  1. Date parameter validity
  2. Positive offset width
 getPositionFromDateOnGantt = computedFn((date: string | Date, offSetWidth: number) => {
   if (!this.currentViewData) return;
+  if (offSetWidth < 0) return;
+  if (typeof date === 'string' && isNaN(Date.parse(date))) return;
 
   return getPositionFromDate(this.currentViewData, date, offSetWidth);
 });

Line range hint 1-324: Consider implementing a Timeline validation service

Given the complexity of timeline calculations and the various edge cases (invalid dates, positions, widths), consider extracting validation logic into a dedicated service. This would:

  1. Centralize validation rules
  2. Make edge cases more discoverable
  3. Simplify testing
web/core/components/issues/issue-layouts/utils.tsx (1)

678-712: Function implementation looks good with room for minor improvements

The function correctly implements the block view details generation with proper handling of partial dates and visual indicators. A few suggestions to enhance maintainability:

  1. Add explicit return type interface
  2. Extract gradient strings as constants
  3. Enhance JSDoc with parameter descriptions

Consider this refactor:

+ interface BlockViewDetails {
+   message: string | undefined;
+   blockStyle: CSSProperties;
+ }
+
+ const GRADIENT_RIGHT = (color: string) => `linear-gradient(to right, ${color} 50%, transparent 95%)`;
+ const GRADIENT_LEFT = (color: string) => `linear-gradient(to left, ${color} 50%, transparent 95%)`;
+
 /**
  * This Method is used to get Block view details, that returns block style and tooltip message
+ * @param block - The block object containing start and target dates
+ * @param backgroundColor - The background color to use for the block
+ * @returns BlockViewDetails object containing message and style properties
  */
 export const getBlockViewDetails = (
   block: { start_date: string | undefined | null; target_date: string | undefined | null } | undefined | null,
   backgroundColor: string
- ) => {
+ ): BlockViewDetails => {
   // ... rest of the function
   if (block?.start_date) {
     message = `From ${renderFormattedDate(block.start_date)}`;
-    blockStyle.maskImage = `linear-gradient(to right, ${backgroundColor} 50%, transparent 95%)`;
+    blockStyle.maskImage = GRADIENT_RIGHT(backgroundColor);
   } else if (block?.target_date) {
     message = `Till ${renderFormattedDate(block.target_date)}`;
-    blockStyle.maskImage = `linear-gradient(to left, ${backgroundColor} 50%, transparent 95%)`;
+    blockStyle.maskImage = GRADIENT_LEFT(backgroundColor);
   }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between db8c4f9 and 2956668.

📒 Files selected for processing (16)
  • web/ce/store/timeline/base-timeline.store.ts (4 hunks)
  • web/core/components/gantt-chart/blocks/block-row.tsx (1 hunks)
  • web/core/components/gantt-chart/blocks/block.tsx (3 hunks)
  • web/core/components/gantt-chart/chart/main-content.tsx (4 hunks)
  • web/core/components/gantt-chart/chart/views/month.tsx (1 hunks)
  • web/core/components/gantt-chart/chart/views/quarter.tsx (1 hunks)
  • web/core/components/gantt-chart/chart/views/week.tsx (1 hunks)
  • web/core/components/gantt-chart/constants.ts (1 hunks)
  • web/core/components/gantt-chart/helpers/blockResizables/use-gantt-resizable.ts (2 hunks)
  • web/core/components/gantt-chart/sidebar/gantt-dnd-HOC.tsx (2 hunks)
  • web/core/components/gantt-chart/sidebar/issues/block.tsx (1 hunks)
  • web/core/components/gantt-chart/sidebar/modules/block.tsx (1 hunks)
  • web/core/components/gantt-chart/views/helpers.ts (2 hunks)
  • web/core/components/issues/issue-layouts/gantt/blocks.tsx (4 hunks)
  • web/core/components/issues/issue-layouts/utils.tsx (3 hunks)
  • web/core/components/modules/gantt-chart/blocks.tsx (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • web/core/components/gantt-chart/chart/views/month.tsx
🔇 Additional comments (27)
web/core/components/gantt-chart/constants.ts (1)

9-9: Verify consistent usage of DEFAULT_BLOCK_WIDTH across components.

Let's ensure this constant is being used consistently for block width calculations throughout the Gantt chart components.

✅ Verification successful

Please update the verification scripts to correctly target TypeScript files.


DEFAULT_BLOCK_WIDTH is used consistently across all components with no hardcoded width values found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check usage patterns of DEFAULT_BLOCK_WIDTH
# Expected: Should be used consistently for block width calculations

# Check for direct usage of the constant
rg "DEFAULT_BLOCK_WIDTH" --type typescript

# Look for any hardcoded values that might need to use this constant instead
ast-grep --pattern 'width: "60px"' 
ast-grep --pattern 'width: 60'

Length of output: 147


Script:

#!/bin/bash
# Description: Check usage patterns of DEFAULT_BLOCK_WIDTH
# Expected: Should be used consistently for block width calculations

# Check for direct usage of the constant in TypeScript files
rg "DEFAULT_BLOCK_WIDTH" --glob "*.ts" --glob "*.tsx"

# Look for any hardcoded values that might need to use this constant instead
rg 'width:\s*"60px"' --glob "*.ts" --glob "*.tsx"
rg 'width:\s*60' --glob "*.ts" --glob "*.tsx"

Length of output: 878

web/core/components/modules/gantt-chart/blocks.tsx (5)

8-12: LGTM: Well-organized imports

The new imports are properly organized and documented with clear grouping comments.


34-37: LGTM: Improved block styling logic

The use of getBlockViewDetails centralizes the styling logic and makes it more maintainable.


Line range hint 71-89: LGTM: ModuleGanttSidebarBlock remains stable

The sidebar block component maintains its clean implementation while complementing the main block's enhancements.


59-65: Verify accessibility of the sticky block name implementation

The sticky block name feature has been implemented as requested, but please verify:

  1. The background overlay's opacity (50%) provides sufficient contrast
  2. The sticky positioning works correctly across different viewport sizes
#!/bin/bash
# Search for related accessibility tests
rg -g '*.test.*' -g '*.spec.*' 'ModuleGanttBlock.*accessibility'

# Check for other uses of similar overlay patterns
ast-grep --pattern 'className="absolute left-0 top-0 h-full w-full bg-custom-background-100/50"'

40-46: Enhanced tooltip content structure

The tooltip now provides better context with a clear hierarchy (name and details).

web/core/components/gantt-chart/blocks/block.tsx (2)

49-50: LGTM! Variables align with Timeline view enhancement requirements.

The new visibility variables correctly implement the requirement to show blocks with either a start date or an end date, while also tracking complete blocks (those with both dates).


92-92: Verify the restriction on block movement.

The change only allows moving blocks that have both start and end dates. Please verify if this restriction aligns with the intended user experience and business requirements.

web/core/components/gantt-chart/chart/views/week.tsx (2)

16-16: LGTM: Improved height management using Tailwind classes.

The change from inline styles to Tailwind's min-h-full class is a good improvement for consistent styling.


13-13: 🛠️ Refactor suggestion

Replace any type with proper interface definition.

Using any as props type reduces type safety and makes the component harder to maintain. Consider defining a proper interface for the component's props.

- export const WeekChartView: FC<any> = observer(() => {
+ interface WeekChartViewProps {
+   // Add required props here based on usage
+ }
+ 
+ export const WeekChartView: FC<WeekChartViewProps> = observer(() => {

Likely invalid or redundant comment.

web/core/components/gantt-chart/chart/views/quarter.tsx (3)

Line range hint 29-46: Well-implemented sticky header with good UX details!

The sticky header implementation is robust with:

  • Proper z-index management
  • Clear visual hierarchy
  • Nice UX touches (current quarter indicator, consistent typography)

Line range hint 82-92: Verify overflow behavior with different content scenarios.

The month columns use overflow-hidden which might clip content in certain scenarios. Consider testing with:

  • Long block names
  • Multiple blocks in the same time period
  • Different zoom levels affecting dayWidth

18-18: LGTM! Verify layout behavior across different viewport sizes.

The switch from explicit minHeight calculation to min-h-full is a good improvement that aligns with modern CSS best practices and makes the height management more flexible.

Let's verify if this change affects other chart views:

✅ Verification successful

Height management changes are consistently applied across all chart views. No issues found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if similar height management is consistent across other chart views
# and if there are any remaining explicit HEADER_HEIGHT calculations

# Search for explicit height calculations in chart views
rg "minHeight.*HEADER_HEIGHT" "web/core/components/gantt-chart/chart/views/"

# Search for min-h-full usage in other chart views for consistency
rg "min-h-full" "web/core/components/gantt-chart/chart/views/"

Length of output: 1540


Script:

#!/bin/bash
# Description: Check if similar height management is consistent across other chart views
# and if there are any remaining explicit HEADER_HEIGHT calculations

# Search for explicit height calculations in chart views
rg "minHeight.*HEADER_HEIGHT" "web/core/components/gantt-chart/chart/views/"

# Search for min-h-full usage in other chart views for consistency
rg "min-h-full" "web/core/components/gantt-chart/chart/views/"

Length of output: 522

web/core/components/gantt-chart/sidebar/gantt-dnd-HOC.tsx (1)

Line range hint 1-108: Verify component usage in sticky block name feature.

The changes look good for improving drag-and-drop safety, but let's verify how this component integrates with the sticky block name feature mentioned in the PR objectives.

web/core/components/gantt-chart/views/helpers.ts (2)

2-2: LGTM! Good practice to centralize constants.

The import of DEFAULT_BLOCK_WIDTH from constants follows good practices for maintainability.


85-95: LGTM! Enhanced date handling aligns with PR objectives.

The changes successfully implement the requirement to support blocks with either start or end date, improving Timeline view flexibility.

Also applies to: 97-97

web/core/components/gantt-chart/blocks/block-row.tsx (1)

Line range hint 1-124: Implementation successfully meets PR objectives.

The changes effectively implement both key requirements:

  1. Sticky positioning for block names via the scroll button
  2. Flexible date requirements allowing blocks with either start or end date

The code is well-structured and maintains good separation of concerns.

web/core/components/issues/issue-layouts/gantt/blocks.tsx (4)

7-8: LGTM: Import changes align with new features

The new imports support the sticky block name feature and enhanced block visualization.

Also applies to: 18-19


50-75: LGTM: Improved block visualization with sticky names

The implementation successfully:

  • Makes block names sticky using SIDEBAR_WIDTH
  • Enhances UX with informative tooltips
  • Provides clear visual feedback through block styling

99-103: LGTM: Improved event handling

The click handler correctly prevents event propagation and default behavior before navigation.


45-45: Verify date handling implementation

Let's verify that the getBlockViewDetails utility correctly handles blocks with either start or end dates.

✅ Verification successful

getBlockViewDetails correctly handles blocks with start and/or end dates

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the getBlockViewDetails implementation
ast-grep --pattern 'export const getBlockViewDetails = $_' -A 20

Length of output: 2704

web/core/components/gantt-chart/chart/main-content.tsx (2)

111-118: LGTM: Enhanced scroll detection logic

The improved scroll handling with dual conditions ensures more reliable detection of when to load additional content, providing a better infinite scrolling experience.


Line range hint 31-199: Well-implemented timeline enhancements

The changes effectively implement the PR objectives:

  1. Support for blocks with either start or end dates
  2. Enhanced scroll handling for better user experience
  3. Consistent layout adjustments

The implementation is clean, maintainable, and follows React best practices.

web/ce/store/timeline/base-timeline.store.ts (3)

8-12: LGTM: Clean import organization

The new imports are well-organized and logically grouped with related functionality.


54-54: LGTM: Well-defined interface method

The new method signature is clear, type-safe, and consistent with the interface's existing pattern.


194-194: Verify edge cases with the relaxed condition

The condition now allows updates when either startDate OR dayWidth is present. While this adds flexibility, please verify:

  1. That getItemPositionWidth handles cases where only one value is present
  2. That this doesn't lead to invalid position calculations
web/core/components/issues/issue-layouts/utils.tsx (1)

36-36: LGTM: Import statement is correctly placed

The import of renderFormattedDate is appropriately placed with other helper imports and is necessary for the new block view functionality.

@pushya22 pushya22 merged commit 4b50b27 into preview Nov 13, 2024
22 of 23 checks passed
@pushya22 pushya22 deleted the minor-timeline-enhancements branch November 13, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛bug Something isn't working 🌟enhancement New feature or request 🌐frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants