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.

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

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

merged 4 commits into from
Jul 31, 2017

Conversation

AdamWard1995
Copy link
Contributor

This project uses semver, please check the scope of this pr:

  • #none# - documentation fixes and/or test additions
  • #patch# - backwards-compatible bug fix
  • #minor# - adding functionality in a backwards-compatible manner
  • #major# - incompatible API change

This PR prevents text from being selected when doing a range select and when clicking on a row body (as opposed to the checkbox) that row will be the only selected one

CHANGELOG

  • Fixed selection functionality to match ember-frost-list

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b97848c on AdamWard1995:fix_selection into ** on ciena-frost:master**.

Copy link
Contributor

@job13er job13er left a comment

Choose a reason for hiding this comment

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

I don't use table selection, and am not familiar with the implementation/behavior in frost-list, so I'm probably not the best reviewer for this. Looks like @quincyle is the biggest contributor on ember-frost-list, so maybe he can take a peek.

},

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

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 01d052d on AdamWard1995:fix_selection into ** on ciena-frost:master**.

Copy link
Contributor

@quincyle quincyle left a comment

Choose a reason for hiding this comment

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

The code change looks fine. Can we get some tests for the range selection?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b1e11f3 on AdamWard1995:fix_selection into ** on ciena-frost:master**.

@quincyle
Copy link
Contributor

quincyle commented Jul 31, 2017

Approved

Approved with PullApprove

@quincyle quincyle merged commit aecbe0d into ciena-frost:master Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants