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

When itemKey attributes is omitted, can't deselect the selected row. #52

Open
quincyle opened this issue Feb 2, 2018 · 4 comments
Open

Comments

@quincyle
Copy link
Contributor

quincyle commented Feb 2, 2018

unable_to_deselect_row

@juwara0
Copy link
Contributor

juwara0 commented Feb 2, 2018

@quincyle - Thank you for opening an issue to track this. Can you provide the following details about this issue:

  • How can this be reproduced?
  • Where is this occurring in an app or when used inside another add-on?
  • What version of Ember CLI are you using when consuming this add-on?
  • What version of Ember are you using when consuming this add-on?
  • What version of this repo (ember-frost-table) are you using?
    • Please run npm ls <package-name> in the terminal in the directory for the code base where you are seeing this to gather that information.
  • Can you point us to an instance of code where this is occurring so we can see the full setup that is in place?

@quincyle
Copy link
Contributor Author

quincyle commented Feb 2, 2018

  • Description:
    Found the issue when installing ember-frost-table: 2.0.0 to an app. This can be reproduced by removing itemKey API from any demo code of dummy app.

  • Env
    ember-cli: 2.12.3,
    ember-source: 2.12.2
    node: v8.9.0,

  • Solution:
    Make itemKey optional for the selection diff comparison algorithm.

  • Walkaround:
    Always provides itemKey.

@notmessenger
Copy link
Contributor

@quincyle itemKey is a required property for the Selection API (https://ciena-frost.github.io/ember-frost-table/#/selection) to function correctly (many occurrences of itemKey in this code -

select (isRangeSelect, isSpecificSelect, item, itemKey, allItems, selectedItems, rangeState) {
)

How do you propose maintaining the same functionality with this value not being set? Do you have a specific scenario in which the population of the itemKey property is problematic for you?

As it currently stands this issue is not one that will be addressed but I am open to it being shown to me what I am not understanding about the situation.

@quincyle
Copy link
Contributor Author

quincyle commented Feb 2, 2018

I don't have a problem to populate an itemKey and feed it into table, but the circumstance of it is being required doesn't seem correct to me.

itemKey was initially introduced to solve the problem of shallow comparison. Imaging we have record1 being selected, later on, the UI receives a new copy of record1 via polling, but these two objs are not the same object from javascript perspective. Now we have a hard time to compare them and retain the selection state of UI.

But thanks for ember-data, we don't need to deal with this headache very often as ember-data wraps the response coming from the server in its record model and reuse the same object before and after the polling.

The itemKey concept was first implemented in ember-frost-list. By default, list will perform a shallow comparison, but we also expose the configurable itemKey, so only list records with a matching itemKey will update when itemKey is set.

This behavior changes when it gets pulled into ember-frost-table, as itemKey becomes a required field for selection to work. But as @notmessenger you mentioned, this is actually indicated in the doc of ember-frost-table. (I didn't notice it) So I will consider it as an inconsistent behavior instead of a bug.

I can raise a PR to make it align with ember-frost-list, or we can leave it as so.

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

No branches or pull requests

3 participants