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
|
Norkz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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. |
WalkthroughCollapsible group functionality was introduced to both desktop and mobile table group components, enabling users to expand or collapse groups with persistent state stored in sessionStorage. Accessibility and keyboard support were added to the toggle controls. Group header icon rendering was removed from group title components. Unit tests for the collapse feature were also introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableGroup
participant sessionStorage
User->>TableGroup: Clicks group toggle button
TableGroup->>TableGroup: _toggleCollapse()
TableGroup->>sessionStorage: Save collapsed state (by view ID & group key)
TableGroup->>TableGroup: Update collapsed$ signal
TableGroup-->>User: Rerender group (collapsed or expanded)
sequenceDiagram
participant User
participant MobileTableGroup
participant sessionStorage
User->>MobileTableGroup: Clicks group toggle button
MobileTableGroup->>MobileTableGroup: _toggleCollapse()
MobileTableGroup->>sessionStorage: Save collapsed state (by view ID & group key)
MobileTableGroup->>MobileTableGroup: Update collapsed$ signal
MobileTableGroup-->>User: Rerender group (collapsed or expanded)
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)
✨ 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## canary #12671 +/- ##
===========================================
- Coverage 56.17% 44.44% -11.73%
===========================================
Files 2662 414 -2248
Lines 127567 51998 -75569
Branches 20091 787 -19304
===========================================
- Hits 71659 23110 -48549
+ Misses 54278 28866 -25412
+ Partials 1630 22 -1608
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
…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. |
forehalo
force-pushed
the
canary
branch
4 times, most recently
from
1c0c9fc to
8d77c48
Compare
You can use |
|
Will change it to that then. And fix the issue that made the server test fail. |
The test errors on the server may not have been caused by this PR. We can check if they occur again in the next CI run. |
|
Understood. |
For some reason that icon creates a duplicate of the group when the dropdown is open. Strange.
|
|
I fixed it and will soon commit. |
…n, and ToggleRightIcon for better readability and consistency
|
This PR is now complete. |
feat_group-by-collapse-toggle.mp4
Adds a collapsible toggle for group-by groups.
Summary by CodeRabbit
New Features
Bug Fixes
Tests