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

bug fix: getCell returned "undefined" when it was expected to return -2 or -4 #6079

Merged
merged 4 commits into from Jul 15, 2019

Conversation

@warpech
Copy link
Contributor

commented Jul 2, 2019

Context

This PR fixes a blocking issue that I discovered while working on #6064

How has this been tested?

I added more assertions to the test of getCell, which indirectly tests getLastRenderedRow and getLastRenderedColumn.

However, it does not test getLastVisibleRow and getLastVisibleColumn. These methods were also fixed in this PR and I need to ask someone for help in writing tests for these changes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

Checklist:

  • My code follows the code style of this project,
  • My change requires a change to the documentation.
bug fix: getCell returned "undefined" when it was expected to return …
…-2 or -4

because the methods "getLastRenderedRow" and "getLastRenderedColumn" were incorrectly using rendered index in a comparison where source index was expected

in this commit I am also fixing "getLastVisibleRow" and "getLastVisibleColumn", but I am not adding any tests for it. I am sorry, these methods are not already tested and I don't have more time to work on this. I think that tests for these methods should be added

@warpech warpech changed the title bug fix: getCell returned "undefined" when it was expected to return … bug fix: getCell returned "undefined" when it was expected to return -2 or -4 Jul 2, 2019

@budnix budnix self-requested a review Jul 9, 2019

@budnix budnix self-assigned this Jul 9, 2019

budnix added some commits Jul 10, 2019

Add missing tests for table.js methods
Add tests for particular methods: `isLastColumnFullyVisible`,
`getFirstVisibleRow`, `getLastVisibleRow`, `getFirstVisibleColumn`,
`getLastVisibleColumn`, `getFirstRenderedRow`, `getLastRenderedRow`,
`getFirstRenderedColumn`, `getLastRenderedColumn`,
`getVisibleRowsCount`, `getVisibleColumnsCount`, `getRenderedRowsCount`,
`getRenderedColumnsCount`.
@budnix

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

I've reviewed the changes and it seems to be good. I added missing tests for mentioned methods and more. After syncing with develop branch there is a problem with UndoRedo plugin which fails in one test - this issue will be fixed in a separate branch.

I assigned @jansiegel to review my changes.

@budnix budnix requested review from jansiegel and budnix and removed request for budnix Jul 10, 2019

@budnix budnix assigned jansiegel and unassigned budnix Jul 10, 2019

@budnix

budnix approved these changes Jul 11, 2019

@warpech

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

Thanks for the help with tests, merging!

@warpech warpech merged commit f3d59dc into develop Jul 15, 2019

4 checks passed

continuous-integration/codeship Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk - package.json (krzysztofspilka) No manifest changes detected

@warpech warpech deleted the fix-getCell branch Jul 15, 2019

@jansiegel jansiegel added this to the July 2019 milestone Jul 31, 2019

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