-
Notifications
You must be signed in to change notification settings - Fork 0
VC-4380 Rewrite Column Moving to improve user experience #17
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
Conversation
@@ -201,6 +201,7 @@ exports.mixin = { | |||
this.behavior.changed(); | |||
var oldX = this.hScrollValue; | |||
this.hScrollValue = x; | |||
this.sbHScroller.index = x; |
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 is required to update the value in the scroller so the grid doesnt randomly jump back to the old position after a shape change
@@ -1433,7 +1433,7 @@ var defaults = { | |||
* @default | |||
* @memberOf module:defaults | |||
*/ | |||
columnGrabMargin: 5, | |||
columnGrabMargin: 10, |
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 makes it a little easier to drag columns to move them or remove them.
We should change the behavior completely though going forward. This is required right now as both the ColumnSelection and ColumnMoving features depend on the mouse down event - this grab margin effectively splits the header cell into two regions, the top pixels are where the column is grabbed, the bottom is where its selected.
A more sensible approach would be for the ColumnSelection feature only select on the mouse up event (instead of down), and to not select if up event is on a different cell than the down event. That way both the ColumnSelection and ColumnMoving could operate on the whole column header.
@@ -0,0 +1,156 @@ | |||
import { IFeature } from "./IFeature"; |
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 is doing the same as Feature.js. It allows us to make typescript features that behave the same way as the old js ones.
Eventually the Feature.js will be replaced with this when all Features are migrated to TypeScript.
@@ -32,7 +32,7 @@ var Registry = Base.extend('Registry', { | |||
} | |||
|
|||
// getClassName defined if new item derived from extend-me | |||
name = name || item.getClassName && item.getClassName(); | |||
name = name || item.getClassName && item.getClassName() || item.name; |
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.
TypeScript features use item.name to get the name of the feature
@@ -91,7 +91,7 @@ var ColumnSelection = Feature.extend('ColumnSelection', { | |||
!event.primitiveEvent.detail.isRightClick && | |||
( | |||
grid.properties.autoSelectColumns || | |||
event.isHeaderCell && event.mousePoint.y >= 5 | |||
event.isHeaderCell && event.mousePoint.y >= grid.properties.columnGrabMargin |
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.
Fix to use the columnGrapMargin instead of hard coding 5 in here
This PR rewrites the ColumnMoving feature to improve UX.
The main change is that we only show an indication of where the column will be dropped as we are dragging, and do a single move operation on mouse up. The old implementation moved the column as it was dragged, swapping columns as the drag happened.
This feature was written in TypeScript and as part of the change I've migrated some of the classes to TypeScript (with minimal changes to get it compiling).