-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[Table] Add column reorder handle when interaction bar enabled #1250
Conversation
packages/core/src/common/utils.ts
Outdated
/** | ||
* Returns true if at least one item in the array satifies the predicate. | ||
*/ | ||
export function some(a: any[] = [], predicate: (item: any) => boolean) { |
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.
Isn't this a thing that already exists: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some?v=control ?
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.
-.-
// cache for easy access later in the lifecycle | ||
const selectedInterval = isRowHeader ? selectedRegion.rows : selectedRegion.cols; | ||
this.selectedRegionStartIndex = selectedInterval[0]; | ||
// add 1 to correct for the fencepost |
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.
Nitpick -- can you be a little bit more descriptive here? A "because selected interval is inclusive" or "includes its endpoints" or something would be ok; just looking for a why-the-fencepost.
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.
Yep
Fix lintPreview: documentation | table |
@@ -73,6 +77,7 @@ export interface IHeaderCellProps extends IProps { | |||
export interface IInternalHeaderCellProps extends IHeaderCellProps { | |||
/** | |||
* Specifies if the cell is reorderable. | |||
* @deprecated since 1.21.0; pass `isReorderable` to `ColumnHeader` or `RowHeader` instead |
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.
@gscshoyru what do you think about this? I moved the logic for adding the TABLE_HEADER_REORDERABLE
class from here into Header
, so this prop serves no purpose here anymore. See: https://github.com/palantir/blueprint/pull/1250/files#diff-34e8a9161a519e81e1d774377ef4c28cR323
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.
Seems fine
Fix deprecation messagePreview: documentation | table |
@@ -73,6 +77,7 @@ export interface IHeaderCellProps extends IProps { | |||
export interface IInternalHeaderCellProps extends IHeaderCellProps { | |||
/** | |||
* Specifies if the cell is reorderable. | |||
* @deprecated since 1.21.0; pass `isReorderable` to `ColumnHeader` or `RowHeader` instead |
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.
Seems fine
Fixes #1242, Fixes #1173
Checklist
Changes proposed in this pull request:
Add reorder handles to column headers when
useInteractionBar={true}
.mousedown
within the handle will trigger reordering.mousedown
in the entire cell will trigger reordering, as before.mousedown
.Also, update docs to include a new "Reorderable content" section.
Reviewers should focus on:
How does the general architecture look? Feels slightly messy, partly because I opted not to create a separate
ReorderHandle
component, since this logic is used in only one place (ResizeHandle
, on the other hand, is used for both row and column headers.) May be passable though.Screenshot