Skip to content

fix/#746#875

Merged
revolist merged 2 commits into
mainfrom
#746
May 27, 2026
Merged

fix/#746#875
revolist merged 2 commits into
mainfrom
#746

Conversation

@revolist

@revolist revolist commented May 27, 2026

Copy link
Copy Markdown
Owner

Closes #746

Summary by CodeRabbit

  • Bug Fixes

    • Sorting indicators now only appear for headers with an active sorting state.
    • Resize handles are shown only when resizing is enabled for a header.
  • Tests

    • Added end-to-end test verifying header sorting indicators and resize affordances render correctly under different configurations.

Review Change Stack

@revolist revolist requested a review from m2a2x May 27, 2026 14:40
@revolist revolist self-assigned this May 27, 2026
@revolist revolist added bug Something isn't working low Minor improvement/issue labels May 27, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1ea0a091a1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/components/header/header-renderer.tsx Outdated
@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 80cb102a-c5ab-40e2-b13d-df1c0e5d0a27

📥 Commits

Reviewing files that changed from the base of the PR and between 1ea0a09 and 20f7399.

📒 Files selected for processing (2)
  • e2e/layout.spec.ts
  • src/components/header/header-renderer.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/components/header/header-renderer.tsx
  • e2e/layout.spec.ts

Walkthrough

Header column affordances are now conditionally rendered: sorting indicators appear only when column data indicates sorting state, and resize handles render only when a header is active and resizable. An e2e test validates these behaviors by mounting the grid with and without resize enabled.

Changes

Conditional Header UI Affordances

Layer / File(s) Summary
Sorting and resize affordances conditional rendering
src/components/header/header-renderer.tsx, src/components/header/resizable.element.tsx
HeaderRenderer computes hasSortingSign from p.data?.sortable, p.data?.order, and p.data?.sortIndex and renders SortingSign only when true. ResizableElement emits resize handle nodes only when props.active && props.canResize; the prior fallback no-resize handle path was removed.
Test coverage for conditional header affordances
e2e/layout.spec.ts
Adds an end-to-end Playwright test that mounts the grid with resize:false and resize:true to assert the correct presence/absence of .sort-indicator, .sort-off, .resizable-r, and .no-resize elements across configurations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • revolist/revogrid#858: Related changes to building/emitting sorting order that the header renderer now depends on.
  • revolist/revogrid#715: Changes how sorting state is injected into header render data, which affects conditional sort indicator rendering.
  • revolist/revogrid#747: Prior modifications to resizable.element.tsx and non-resizable/resize-handle UI logic.

Suggested reviewers

  • m2a2x

Poem

🐰 I hopped through headers, quiet and neat,
Saw signs that show when columns meet,
Handles appear when both flags say true,
No ghost divs now to bother you—
A tidy grid, from me to you.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The PR title 'fix/#746' is vague and refers only to an issue number without describing the actual change (preventing unnecessary header DOM nodes rendering). Use a descriptive title like 'Prevent rendering unused header DOM nodes for sorting and resize' that clearly explains the change.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed The code changes directly address issue #746: they prevent rendering of unnecessary header DOM nodes (sort indicators and resize handles) when features are disabled, exactly as requested.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing issue #746: conditional rendering of sorting signs and resize handles, plus corresponding test coverage.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch #746

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud

Copy link
Copy Markdown

@revolist revolist merged commit 7140d23 into main May 27, 2026
8 checks passed
@revolist revolist deleted the #746 branch May 27, 2026 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working low Minor improvement/issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Strange behaviour on the header - extra divs when not in used and half behaviour

2 participants