-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Adding MarqueeSelection component and example #74
Conversation
merging latest
…-react into marquee-selection # Conflicts: # src/components/DetailsList/DetailsList.tsx
…n to measures to improve performance.
private _dragOrigin: IPoint; | ||
private _rootRect: IRectangle; | ||
private _lastMouseEvent: MouseEvent; | ||
private _autoScroll; |
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.
Missing type.
* With scrolltop fix. * addressing lint issues
…, due to performance.
/** | ||
* Gives the class constructor, will iterate through prototype methods prefixed with "_on" and bind them to "this". | ||
* Example: in your constructor, you'd have: this.autoBindCallbacks(MyComponent); */ | ||
protected autoBindCallbacks(object: Function) { |
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 think it would make a lot more sense just to define a @bind
decorator:
@bind
private _onClick(event: MouseEvent) {
this.setState({
isActive: true
});
}
The @bind
decorator would cleanly handle the binding once the instance was constructed.
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 like that suggestion. I'll remove it here and approach it that way separately.
* if we should start a marquee selection. If true is returned, we will cancel the mousedown | ||
* event to prevent upstream mousedown handlers from executing. | ||
*/ | ||
onShouldStartSelection?: (ev: React.MouseEvent) => 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.
I think this should be widened or altered.
To control whether or not marquee selection is allowed at all, I think passing an isEnabled
flag makes more sense. However, to control whether or not a current drag gesture should activate a marquee, I think you should pass the proposed initial selection Rectangle
so the owner can decide whether to let selection continue. The MouseEvent
by itself is insufficient.
@bind
private _onShouldStartSelection(area: Rectangle, event: React.MouseEvent): boolean {
if (!this._isDropping) {
return false;
}
if (area.width > 10 || area.height > 10) {
return true;
}
return false;
}
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'm not sure that passing along the rectangle helps at all. It would force the caller to figure out whats in the box, which pushes some of the logic in more than one place. However, it is possible we could try to identify if the pixel you clicked on is an item and pass that as a parameter. That could make it cleaner.
…ixes column draggging to use native eventing to play well with marquee selection.
…c-react into marquee-selection # Conflicts: # src/components/DetailsList/DetailsHeader.tsx
* Adding autoscroll utility and marquee selection. * More fixes. * Enables selection preservation even when items dematerialize. * Math rounding tweak in auto scrolling. * Updating small nits. * Adding example page, improving props documentation, adding memoization to measures to improve performance. * More performance improvements. * Moving files to a more logical location. * Missing an index change. * Adding more best practices content. * Updating documentation. * Removing unnecessary call. * Removing dir from html. * Removing an unnecessary measure from autoscroll. * Updating basic details list example to use marquee selection. * With scrolltop fix (#3) * With scrolltop fix. * addressing lint issues * Improving the example by removing the images. * Removing the scroll monitoring and css tweaking from Fabric component, due to performance. * Fixing issues related to safari support. * Minor improvement to EventGroup. * Lint fixes. * Updates for PR comments. * Fixing hovers. * A few more fixes to test page and styles. * Removing lint error. * Cleanup. * Adds ability to select from anywhere in the scrollable parent. Also fixes column draggging to use native eventing to play well with marquee selection.
This changelist adds MarqueeSelection to the mix of utilities, which gives the user the ability to drag a rectangle around some items to select them.
This utility component allows you to specify a region of content where a MarqueeSelection should exist. It simply requires a Selection object to interact with, and requires the elements representing items in the selection to have data-selection-index attributes defined.