feat(editor): add collapse/expand toggle for groups with caching #12671
Conversation
…ve unused icon rendering and improve code clarity ✨ (group.ts): add toggle functionality for group collapse/expand with visual indicators 📝 (group.ts): update styles for group toggle button and improve layout consistency
|
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
WalkthroughThe changes remove the group icon rendering from group titles in both mobile and desktop components. Additionally, the table group component is updated to support collapsible group rows, introducing a toggle button to expand or collapse group content, along with related UI and control flow adjustments. A new arrow icon is added for the toggle button. Unit tests for collapse functionality and a Vitest configuration file are also introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableGroup
participant UI
User->>TableGroup: Clicks group header toggle button
TableGroup->>TableGroup: Toggle collapsed$ state
TableGroup->>UI: Rerender group rows (show/hide based on collapsed$)
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blocksuite/affine/data-view/src/core/group-by/group-title.ts(2 hunks)blocksuite/affine/data-view/src/view-presets/table/pc/group.ts(7 hunks)
🔇 Additional comments (3)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (3)
110-115: LGTM! Clean state management implementation.The signal-based state management for the collapsed state is well implemented. The toggle method correctly prevents event propagation to avoid unintended side effects.
76-102: Excellent CSS implementation for the toggle button.The CSS styling for the toggle button is well thought out with proper hover effects, smooth transitions, and rotation animations. The transform rotation from -90deg to 0deg provides clear visual feedback for the collapsed/expanded states.
350-354: Efficient conditional rendering implementation.The conditional rendering logic correctly shows/hides the rows based on the collapsed state while always displaying the group header. Using
nothingfrom lit for the collapsed state is the appropriate approach.
…emove redundancy ✨ (group.ts): enhance accessibility by adding ARIA attributes and keyboard support for group toggle button
|
I have finished this PR. Do advise what else needs to be done before this PR can be approved and merged. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (1)
200-211: Accessibility implementation looks comprehensive.The toggle button includes all necessary accessibility attributes and keyboard support as requested in previous reviews.
🧹 Nitpick comments (2)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (2)
184-193: Consider extracting the inline SVG for reusability.The arrow SVG is defined inline within the method, which could impact maintainability if this icon is used elsewhere.
Consider extracting the SVG to a constant or importing from an icon library:
+const ArrowIcon = html`<svg + xmlns="http://www.w3.org/2000/svg" + viewBox="0 0 24 24" + fill="none" +> + <path + fill="currentColor" + d="M13.15 15.132a.757.757 0 0 1-1.3 0L8.602 9.605c-.29-.491.072-1.105.65-1.105h6.497c.577 0 .938.614.65 1.105z" + /> +</svg>`; private readonly renderGroupHeader = () => { if (!this.group) { return null; } - const Arrow = html`<svg - xmlns="http://www.w3.org/2000/svg" - viewBox="0 0 24 24" - fill="none" - > - <path - fill="currentColor" - d="M13.15 15.132a.757.757 0 0 1-1.3 0L8.602 9.605c-.29-.491.072-1.105.65-1.105h6.497c.577 0 .938.614.65 1.105z" - /> - </svg>`;
196-198: Consider moving inline styles to CSS classes.The container uses extensive inline styles which could be moved to CSS classes for better maintainability.
Consider adding a CSS class for the group header container:
+.group-header-container { + position: sticky; + left: 0; + width: max-content; + padding: 6px 0; + margin-bottom: 4px; + display: flex; + align-items: center; + gap: 8px; + max-width: 400px; +} return html` - <div - style="position: sticky;left: 0;width: max-content;padding: 6px 0;margin-bottom: 4px;display:flex;align-items:center;gap: 8px;max-width: 400px" - > + <div class="group-header-container">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blocksuite/affine/data-view/src/core/group-by/group-title.ts(2 hunks)blocksuite/affine/data-view/src/view-presets/table/pc/group.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- blocksuite/affine/data-view/src/core/group-by/group-title.ts
🔇 Additional comments (6)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (6)
7-7: LGTM: Correct import for reactive state management.The signal import is appropriate for managing the collapsed state reactively.
12-12: LGTM: Necessary import for conditional rendering.The
nothingimport is correctly used for conditionally hiding content when groups are collapsed.
76-102: LGTM: Comprehensive styling for toggle functionality.The CSS implementation provides smooth transitions, proper hover states, and intuitive visual feedback with the rotating arrow animation.
110-110: LGTM: Appropriate reactive state initialization.The collapsed signal is correctly initialized to false, making groups expanded by default which follows good UX practices.
112-115: LGTM: Clean toggle implementation.The toggle method correctly prevents event propagation and updates the reactive state.
361-364: LGTM: Proper conditional rendering implementation.The render method correctly shows the group header and conditionally renders rows based on the collapsed state using the
nothingliteral.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## canary #12671 +/- ##
==========================================
- Coverage 55.87% 55.62% -0.25%
==========================================
Files 2652 2652
Lines 125209 125221 +12
Branches 19806 19807 +1
==========================================
- Hits 69958 69655 -303
- Misses 53124 53258 +134
- Partials 2127 2308 +181
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ (table-group.unit.spec.ts): add unit tests for TableGroup collapse functionality ♻️ (group.ts): refactor MobileTableGroup to implement toggle collapse feature with visual feedback ✨ (group.ts): import GroupToggleArrowIcon to replace inline SVG for better maintainability 📝 (vitest.config.ts): add Vitest configuration file to set up testing environment and coverage reporting
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts
Outdated
Show resolved
Hide resolved
…e and PC versions by aligning icon imports
|
Due to adding a new Icon and using |
blocksuite/affine/data-view/src/__tests__/table-group.unit.spec.ts
Outdated
Show resolved
Hide resolved
blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts
Outdated
Show resolved
Hide resolved
…omply with coding standards ♻️ (group.ts): refactor GroupToggleArrowIcon usage to remove unnecessary parentheses for improved readability
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts (2)
7-7: Use consistent import path for the icon.The import path for
GroupToggleArrowIconshould use the standardized alias for consistency with the PC group file.
143-143: Correct the icon usage syntax.The
GroupToggleArrowIconis a template literal constant, not a function, so it should be referenced without parentheses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
blocksuite/affine/components/src/icons/misc.ts(1 hunks)blocksuite/affine/data-view/src/__tests__/table-group.unit.spec.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts(5 hunks)blocksuite/affine/data-view/src/view-presets/table/pc/group.ts(7 hunks)blocksuite/affine/data-view/vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- blocksuite/affine/components/src/icons/misc.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts (2)
blocksuite/affine/components/src/icons/misc.ts (1)
GroupToggleArrowIcon(61-70)blocksuite/affine/data-view/src/core/group-by/group-title.ts (1)
GroupTitle(109-219)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (2)
blocksuite/affine/components/src/icons/misc.ts (1)
GroupToggleArrowIcon(61-70)blocksuite/affine/data-view/src/core/group-by/group-title.ts (1)
GroupTitle(109-219)
🪛 ESLint
blocksuite/affine/data-view/src/__tests__/table-group.unit.spec.ts
[error] 1-6: Run autofix to sort these imports!
(simple-import-sort/imports)
🔇 Additional comments (9)
blocksuite/affine/data-view/src/__tests__/table-group.unit.spec.ts (1)
10-32: Test coverage looks comprehensive for the collapse functionality.The tests effectively verify the toggle behavior for both PC and mobile components, checking the initial state and state transitions after toggling.
blocksuite/affine/data-view/vitest.config.ts (1)
1-25: Well-configured Vitest setup for the data-view module.The configuration appropriately sets up the testing environment with DOM simulation, coverage reporting, and console log filtering. The settings are consistent with modern testing practices.
blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts (3)
81-86: Solid implementation of the collapse toggle functionality.The reactive signal and toggle method are well-implemented with proper event handling and state management.
129-144: Excellent accessibility implementation for the toggle button.The toggle button includes proper ARIA attributes (
aria-expanded,aria-label), keyboard support (Enter and Space keys), and semantic HTML withrole="button"andtabindex="0".
196-201: Clean conditional rendering implementation.The use of Lit's
nothingdirective for conditional rendering when collapsed provides an efficient way to hide/show group content.blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (4)
110-115: Well-structured collapse state management.The reactive signal and toggle method implementation is clean and follows good patterns for state management.
190-203: Accessibility attributes are properly implemented.Contrary to the past review comment, the toggle button includes comprehensive accessibility features:
role="button",aria-expanded,aria-label,tabindex="0", and keyboard event handlers for Enter and Space keys.
76-102: Clean CSS implementation for toggle animation.The CSS provides smooth transitions for the toggle button with proper hover effects and arrow rotation animations.
351-354: Efficient conditional rendering with proper structure.The render method cleanly separates the always-visible group header from the conditionally-rendered rows using Lit's
nothingdirective.
blocksuite/affine/data-view/src/__tests__/table-group.unit.spec.ts
Outdated
Show resolved
Hide resolved
…rganization and readability
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
blocksuite/affine/data-view/src/__tests__/table-group.unit.spec.ts (2)
1-6: Sort imports according to ESLint.
The import statements need to be reordered to satisfy the project'ssimple-import-sortrules.🧰 Tools
🪛 ESLint
[error] 1-6: Run autofix to sort these imports!
(simple-import-sort/imports)
32-33: Add trailing newline at EOF.
Include a newline at the end of the file to comply with POSIX conventions and prevent linting errors.
🧹 Nitpick comments (1)
blocksuite/affine/data-view/src/__tests__/table-group.unit.spec.ts (1)
11-20: Avoid testing private methods directly.
Consider simulating user interactions or using the public API (e.g., triggering the collapse toggle button) instead of calling_toggleCollapseto improve test maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
blocksuite/affine/data-view/src/__tests__/table-group.unit.spec.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
blocksuite/affine/data-view/src/__tests__/table-group.unit.spec.ts
[error] 1-6: Run autofix to sort these imports!
(simple-import-sort/imports)
🔇 Additional comments (2)
blocksuite/affine/data-view/src/__tests__/table-group.unit.spec.ts (2)
8-8: Test environment setup is correct.
The@vitest-environment happy-domdirective is properly configured for DOM-based testing.
22-31: Align element tag name with component definition.
Ensure that the tag'mobile-table-group'matches the registered custom element name inMobileTableGroup; otherwise, the element may not initialize correctly.
… by adding a new icon for group toggling
|
Seems that CI has passed. My concern is: |
zzj3720
changed the title
…or better clarity and performance ♻️ (group.ts): refactor imports to include effect from signals-core for improved functionality
blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts
Outdated
Show resolved
Hide resolved
… consistency style(vitest.config.ts): add a newline at the end of the file to comply with coding standards
NorkzYT
changed the title
…g sessionStorage to persist user preferences chore(tests): update imports in table-group.unit.spec.ts for consistency and clarity chore(package): add vitest as a dependency for testing framework
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Please run CI test. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
blocksuite/affine/data-view/package.json(1 hunks)blocksuite/affine/data-view/src/__tests__/table-group.unit.spec.ts(1 hunks)blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts(5 hunks)blocksuite/affine/data-view/src/view-presets/table/pc/group.ts(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- blocksuite/affine/data-view/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- blocksuite/affine/data-view/src/tests/table-group.unit.spec.ts
- blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (7)
blocksuite/affine/data-view/src/view-presets/table/pc-virtual/row/cell.ts (1)
view(216-218)blocksuite/affine/data-view/src/view-presets/table/pc/cell.ts (1)
view(149-151)blocksuite/affine/data-view/src/view-presets/table/pc/row/row.ts (1)
view(297-299)blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts (1)
view(269-271)blocksuite/affine/data-view/src/view-presets/table/pc/controller/selection.ts (1)
view(103-105)blocksuite/affine/data-view/src/view-presets/table/stats/column-stats-bar.ts (1)
view(46-48)blocksuite/affine/components/src/icons/misc.ts (1)
GroupToggleArrowIcon(61-70)
🔇 Additional comments (2)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (2)
231-244: Nice job adding comprehensive accessibility hooks –role,aria-expanded,aria-label,tabindex, and keyboard activation cover the major requirements.
6-6:❓ Verification inconclusive
Confirm the new icon import path actually resolves
The import now targets
@blocksuite/affine-components/icons, but previous discussion indicated resolution issues. Please make sure the package exportsGroupToggleArrowIcon.
🏁 Script executed:
#!/bin/bash # Verify the symbol can be resolved in the repo rg --files -g '*icons*' | xargs rg -n 'export .*GroupToggleArrowIcon'Length of output: 71
Verify the
GroupToggleArrowIconexport
A local search didn’t find anyexport { GroupToggleArrowIcon }under@blocksuite/affine-components/icons. Please confirm that the published package actually exports this symbol before merging.
- If you control the
affine-componentspackage, ensureGroupToggleArrowIconis included in itsiconsentrypoint (e.g., inpackages/affine-components/icons/src/index.ts).- Otherwise, inspect the distributed files in
node_modules/@blocksuite/affine-components/iconsor the package’s documentation to verify the export.
…16px for better mobile and PC responsiveness
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (2)
75-78:⚠️ Potential issueSelector still has the stray dot – rule never matches
The leading dot turns
affine-data-view-table-group(a custom element) into a class selector, so the hover color is never applied. This was called out in a previous review and is still unresolved.- .affine-data-view-table-group:hover svg { + affine-data-view-table-group:hover svg {
388-396:⚠️ Potential issueCollapsed state can still become stale on reused elements
The earlier feedback about resetting
storageLoadedwhengrouportableViewLogicchanges hasn’t been applied. Without the reset, recycled elements preserve the previous group’s state.- if ( - !this.storageLoaded && - (changed.has('group') || changed.has('tableViewLogic')) - ) { - this._loadCollapsedState(); - } + if (changed.has('group') || changed.has('tableViewLogic')) { + this.storageLoaded = false; + this._loadCollapsedState(); + }
🧹 Nitpick comments (2)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (2)
6-10: Consolidate icon imports to avoid module duplication
GroupToggleArrowIconis imported from@blocksuite/affine-components/icons, whilePlusIconcomes from@blocksuite/icons/lit. Unless there is a compelling reason to load two different icon bundles, pull both from the same barrel file to reduce bundle size and keep tree-shaking effective.-import { GroupToggleArrowIcon } from '@blocksuite/affine-components/icons'; -import { PlusIcon } from '@blocksuite/icons/lit'; +import { PlusIcon, GroupToggleArrowIcon } from '@blocksuite/icons/lit';Verify that the barrel export for
GroupToggleArrowIconexists in@blocksuite/icons/lit; otherwise, add it and adjust the path once.
145-147: Broaden the event type for_toggleCollapse
_toggleCollapseis used for bothclick(MouseEvent) andkeydown(KeyboardEvent).
Typing the parameter asMouseEventis misleading and forces the cast in the key handler. Use the commonEventbase (or a union) instead.-private readonly _toggleCollapse = (e?: MouseEvent) => { +private readonly _toggleCollapse = (e?: Event) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts(5 hunks)blocksuite/affine/data-view/src/view-presets/table/pc/group.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (5)
blocksuite/affine/data-view/src/view-presets/table/mobile/group.ts (1)
view(269-271)blocksuite/affine/data-view/src/view-presets/table/pc-virtual/controller/selection.ts (1)
view(103-105)blocksuite/affine/data-view/src/view-presets/table/pc/controller/selection.ts (1)
view(103-105)blocksuite/affine/data-view/src/view-presets/table/stats/column-stats-bar.ts (1)
view(46-48)blocksuite/affine/components/src/icons/misc.ts (1)
GroupToggleArrowIcon(61-70)
🔇 Additional comments (1)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (1)
231-247: Accessibility & keyboard support look goodARIA attributes,
role="button",tabindex, and Enter/Space handling are correctly implemented.
…in table view style(vitest.config.ts): format configuration file for better readability and consistency
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (2)
75-78: 🛠️ Refactor suggestionSelector still starts with a dot – rule targets nothing
The earlier feedback to drop the leading dot so the selector actually matches the custom element hasn’t been applied.- .affine-data-view-table-group:hover svg { + affine-data-view-table-group:hover svg {
392-400: 🛠️ Refactor suggestionStale collapse state when
group/tableViewLogicchanges
The earlier recommendation to resetstorageLoadedon prop changes hasn’t been implemented; the guard still shortcuts after the first load. Users can see an outdated collapsed state when the component is re-used for a different group.- if ( - !this.storageLoaded && - (changed.has('group') || changed.has('tableViewLogic')) - ) { - this._loadCollapsedState(); - } + if (changed.has('group') || changed.has('tableViewLogic')) { + this.storageLoaded = false; + this._loadCollapsedState(); + }
🧹 Nitpick comments (2)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts (2)
28-49: DuplicatesessionStoragehelper – move to shared util
ThecollapsedStatehelper is now present in both PC & mobile group components. Duplicating logic increases maintenance risk (e.g., keys diverge). Extract it to a small shared module and import it from both places.
239-244: Keyboard handler doesn’t propagate the event object
_toggleCollapsecallse?.stopPropagation(), but the key-down handler invokes it without passing the event, so keyboard interactions won’t stop propagation. Passefor parity with mouse behaviour.- this._toggleCollapse(); + this._toggleCollapse(e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blocksuite/affine/data-view/src/view-presets/table/pc/group.ts(7 hunks)blocksuite/affine/data-view/vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- blocksuite/affine/data-view/vitest.config.ts
|
Complete. |
feat_group-by-collapse-toggle.mp4
Adds a collapsible toggle for group-by groups.
Summary by CodeRabbit
New Features
Style
Bug Fixes
Tests