Skip to content

add header disabled to row markers#981

Merged
lukasmasuch merged 5 commits intoglideapps:mainfrom
BrianHung:header-disabled-row-markers
Jun 25, 2025
Merged

add header disabled to row markers#981
lukasmasuch merged 5 commits intoglideapps:mainfrom
BrianHung:header-disabled-row-markers

Conversation

@BrianHung
Copy link
Collaborator

@BrianHung BrianHung commented Jul 12, 2024

Closes #940

@BrianHung BrianHung force-pushed the header-disabled-row-markers branch from 9d0fefc to 3875ab0 Compare July 12, 2024 06:31

const [hCol, hRow] = hoveredItem ?? [];
const headerHovered = hCol !== undefined && hRow === -1;
const headerHovered = hCol !== undefined && hRow === -1 && columns[hCol].headerRowMarkerDisabled !== true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it even to add this here? I think the current implementation of headerRowMarkerDisabled already prevents the row marker hover (but haven't verified this).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

headerRowMarkerDisabled hides the row marker checkbox, but the header cell still receives hover styles, shows a pointer cursor, and triggers select-all behavior on click.

}
const selected = selection.columns.hasIndex(c.sourceIndex);
const noHover = dragAndDropState !== undefined || isResizing;
const noHover = dragAndDropState !== undefined || isResizing || c.headerRowMarkerDisabled === true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as the comment above. Not sure if this is event required since the exising logic of headerRowMarkerDisabled should already prevent the hover checkbox.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only hides but doesn't prevent interaction; see comment above.

@lukasmasuch lukasmasuch requested a review from Copilot June 25, 2025 15:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds a new flag to disable header interactions for row markers, addressing issue #940. The changes update the header rendering logic, adjust hover behavior in the DataGrid component, and expose a new "headerDisabled" property in both the DataEditor and story examples.

  • Updated data-grid-render to check the headerDisabled flag during header rendering.
  • Modified DataGrid hover logic to respect the headerDisabled setting.
  • Enhanced DataEditor and its story configuration to support header disabling.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
packages/core/src/internal/data-grid/render/data-grid-render.header.ts Updated header drawing logic to skip hover effects when header markers are disabled.
packages/core/src/internal/data-grid/data-grid.tsx Modified headerHovered condition to consider headerDisabled flags.
packages/core/src/docs/examples/row-markers.stories.tsx Added headerDisabled property to story args and interface.
packages/core/src/data-editor/data-editor.tsx Updated row marker logic to incorporate the headerDisabled flag in determining header marker interactivity.
Comments suppressed due to low confidence (1)

packages/core/src/internal/data-grid/render/data-grid-render.header.ts:80

  • [nitpick] Add an inline comment to explain the purpose of the check for 'c.headerRowMarkerDisabled === true' to aid future maintainers in understanding the header hover logic.
        const noHover = dragAndDropState !== undefined || isResizing || c.headerRowMarkerDisabled === true;

Copy link
Collaborator

@lukasmasuch lukasmasuch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@lukasmasuch lukasmasuch merged commit a7b47d8 into glideapps:main Jun 25, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ability to remove checkbox in rowMarkers header

2 participants

Comments