Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make selection behaviour match ember-frost-list #28

Merged
merged 4 commits into from
Jul 31, 2017
Merged

Make selection behaviour match ember-frost-list #28

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b97848c
Make selection behaviour match ember-frost-table
AdamWard1995 Jul 24, 2017
a303656
Fix review comment
AdamWard1995 Jul 26, 2017
01d052d
Merge branch 'master' into fix_selection
AdamWard1995 Jul 27, 2017
b1e11f3
Add more selection tests
AdamWard1995 Jul 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions addon/components/frost-fixed-table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export default Component.extend({

// == Keyword Properties ====================================================

classNameBindings: ['_isShiftDown:shift-down'],
layout,

// == PropTypes =============================================================
Expand All @@ -32,6 +33,7 @@ export default Component.extend({
onSelectionChange: PropTypes.func,

// state
_isShiftDown: PropTypes.bool,
_itemComparator: PropTypes.func,

_rangeState: PropTypes.shape({
Expand Down Expand Up @@ -421,10 +423,26 @@ export default Component.extend({
}
},

setShift (event) {
if (!this.isDestroyed) {
this.set('_isShiftDown', event.shiftKey)
}
},

// == DOM Events ============================================================

// == Lifecycle Hooks =======================================================

init () {
this._super(...arguments)
this._keyHandler = this.setShift.bind(this)
$(document).on(`keyup.${this.elementId} keydown.${this.elementId}`, this._keyHandler)
},

willDestroy () {
$(document).off(`keyup.${this.elementId} keydown.${this.elementId}`, this._keyHandler)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we release the bind on the destroy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call this._super?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitiely missing call to _super, but not sure what you mean by if we should release the binding on destroy. This is what we do in ember-frost-list https://github.com/ciena-frost/ember-frost-list/blob/master/addon/components/frost-list.js#L162 and it seems like the logical hook to do this https://guides.emberjs.com/v2.14.0/components/the-component-lifecycle/#toc_detaching-and-tearing-down-component-elements-with-code-willdestroyelement-code

},

/**
* Set up synced scrolling as well as calculating padding for middle sections
*/
Expand Down
19 changes: 16 additions & 3 deletions addon/components/frost-table-row.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@
*/

import Ember from 'ember'
const {isEmpty} = Ember
const {ViewUtils, isEmpty} = Ember
const {isSimpleClick} = ViewUtils
import computed, {readOnly} from 'ember-computed-decorators'
import {Component} from 'ember-frost-core'
import {ColumnPropType, ItemPropType} from 'ember-frost-table/typedefs'
import {PropTypes} from 'ember-prop-types'

import layout from '../templates/components/frost-table-row'
import {click} from '../utils/selection'

export default Component.extend({
// == Dependencies ==========================================================
Expand Down Expand Up @@ -64,7 +64,20 @@ export default Component.extend({
// == DOM Events ============================================================

click (event) {
click(event, this.onSelect, this.get('item'))
const isRangeSelect = event.shiftKey
const isSpecificSelect = false

// Only process simple clicks or clicks with the acceptable modifiers
if (isSimpleClick(event) || isRangeSelect) {
event.preventDefault()
event.stopPropagation()

this.onSelect({
isRangeSelect,
isSpecificSelect,
item: this.get('item')
})
}
},

// == Lifecycle Hooks =======================================================
Expand Down
12 changes: 12 additions & 0 deletions addon/components/frost-table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* global $ */
/**
* Component definition for the frost-table component
*/
Expand Down Expand Up @@ -118,6 +119,17 @@ export default Component.extend({

// == Lifecycle Hooks =======================================================

init () {
this._super(...arguments)
this._keyHandler = this.setShift.bind(this)
$(document).on(`keyup.${this.elementId} keydown.${this.elementId}`, this._keyHandler)
},

willDestroyElement () {
this._super(...arguments)
$(document).off(`keyup.${this.elementId} keydown.${this.elementId}`, this._keyHandler)
},

didRender () {
const selectable = this.get('_isSelectable')
let totalWidth = 0
Expand Down
5 changes: 5 additions & 0 deletions app/styles/ember-frost-table/_frost-fixed-table.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,4 +142,9 @@ $frost-fixed-table-selected-cell-border-width: 1px;
flex: 1 0 auto;
vertical-align: middle;
}

&.shift-down {
// Range select doesn't select text
user-select: none;
}
}