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

Is getCellMeta using physical or logical row indexes? #3025

Closed
thbar opened this issue Dec 1, 2015 · 8 comments

Comments

@thbar
Copy link

commented Dec 1, 2015

The documentation here does not mention if the row index to be passed has to be physical (= index in the underlying source) or logical (= index in the visual representation, which may be different when some sorting is applied).

Based on my test it looks like it is a logical index, which I'm converting to physical index using this:

logicalToPhysical: (logical_row_index)
  # https://github.com/handsontable/handsontable/issues/2718#issuecomment-139201814
  physical_row_index = Handsontable.hooks.run(@hot, 'modifyRow', logical_row_index)
  physical_row_index

Can anyone confirm that indeed the API contract here is that the row is expected to be a logical index?

I would like to make sure the underlying assumptions do not change in the future. Thanks!

@AMBudnik

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2015

The ones you deliver are logical indexes and ones that are returned in row, col properties are physical. We're not planning to change that in nearest future.

@thbar

This comment has been minimized.

Copy link
Author

commented Dec 1, 2015

Great, thanks!

@thbar

This comment has been minimized.

Copy link
Author

commented Dec 2, 2015

@AMBudnik to clarify - in the case of cells, the row seems to be physical, is that what you meant? Thanks!

@AMBudnik

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2015

Hi @thbar sorry for a delay. I had to chat with other team member to be sure.

At this demo we can see that cells get physical indexes (Nissan as a part of data has row index 0 and it's the same index for readOnly row)

@thbar

This comment has been minimized.

Copy link
Author

commented Dec 8, 2015

Thanks for the clarification - no worries on the delay!

@thbar

This comment has been minimized.

Copy link
Author

commented Dec 8, 2015

Ultimately I think keeping some convention in the documentation to tell if it's physical vs logical could be useful, in order to avoid any mistake that could hurt pretty bad once sorting is activated!

@AMBudnik

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2015

OK, thanks for suggestion.

@swistach swistach self-assigned this May 30, 2017

@swistach swistach added this to the 0.32.1 milestone May 30, 2017

swistach added a commit that referenced this issue May 31, 2017
swistach added a commit that referenced this issue Jun 1, 2017

@swistach swistach assigned wszymanski and unassigned swistach Jun 1, 2017

swistach added a commit that referenced this issue Jun 1, 2017

@wszymanski wszymanski assigned swistach and unassigned wszymanski Jun 12, 2017

swistach added a commit that referenced this issue Jun 12, 2017

@swistach swistach assigned wszymanski and unassigned swistach Jun 12, 2017

@wszymanski wszymanski removed their assignment Jun 12, 2017

@AMBudnik

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2017

Now it looks so much better. Thanks for an update @swistach and thank you @thbar for sharing the issue in the first place

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.