-
Notifications
You must be signed in to change notification settings - Fork 55
[WC-3027] Add Multi-Page Select All functionality to Datagrid #1906
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
base: main
Are you sure you want to change the base?
Conversation
979dd4f
to
688c166
Compare
a72742a
to
692515b
Compare
packages/pluggableWidgets/datagrid-web/src/components/loader/SpinnerLoader.tsx
Outdated
Show resolved
Hide resolved
} | ||
|
||
#{$root}-clear-selection { | ||
#{$root}-btn-invisible { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not really invisible, it makes them look like a link, I would call it btn-link
or something.
packages/pluggableWidgets/datagrid-web/src/components/CheckboxColumnHeader.tsx
Show resolved
Hide resolved
import { createElement } from "react"; | ||
import { useDatagridRootScope } from "../helpers/root-context"; | ||
|
||
export const SelectAllBar = observer(function SelectAllBar(): React.ReactNode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import ReactNode
from "react". We disallowed global React.*
namespace in out React 19 migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
0471555
to
0f52cb0
Compare
performance.mark("DSExportRequest_end"); | ||
const measure = performance.measure("DSExportRequest", "DSExportRequest_send", "DSExportRequest_end"); | ||
console.debug(`DSExportRequest: export took ${(measure.duration / 1000).toFixed(2)} seconds`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some leftovers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep it.
<property key="clearSelectionCaption" type="textTemplate"> | ||
<caption>Clear selection caption</caption> | ||
<description /> | ||
<translations> | ||
<translation lang="en_US">Clear selection</translation> | ||
</translations> | ||
</property> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gallery probably needs a changlog entry about this change
* This method is a hack to reload selection. To work it requires at leas one object. | ||
* The problem is that if we setting value equal to current selection, then prop is | ||
* not reloaded. We solve this by setting ether empty array or array with one object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What use case is this solving, why do we need to reload the prop?
const measure1 = performance.measure("Measure1", "SelectAll_Start", "SelectAll_End"); | ||
console.debug(`Data grid 2: 'select all' took ${(measure1.duration / 1000).toFixed(2)} seconds.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep this in production code? Likely useless for production, but if we want to keep this for dev build mayse there is a way to strip this away via rollup for production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always see debug info for nanoflows. So I think it's fine to keep this one as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logs will be off in production
} | ||
|
||
get clearSelectionText(): string { | ||
return this.gate.props.clearSelectionCaption?.value ?? "clear.selection.caption"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We likely want sensible fallback similar to singular
and plural
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's "clear" text, there is no plural form I think
0f52cb0
to
eebfcd4
Compare
Pull request type
New feature (non-breaking change which adds functionality)
Description
This PR implements multi-page select all functionality for the Datagrid2 widget, allowing users to select all items across multiple pages based on the current filter. The feature includes configurable batch processing, progress tracking, and cancellation support, similar to the existing Excel export functionality.
What should be covered while testing?
Small dataset (< buffer size): Should use immediate selection
Large dataset (>= buffer size): Should show progress dialog and batch processing
Unknown total count: Should fall back to per-page selection
Cancellation: Should properly cancel and restore state
Page restoration: Should return user to original page after selection
Filtered data: Should only select items matching current filter