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

Fix #3405. Updated react-data-grid to latest version #3438

Merged
merged 2 commits into from
Jan 10, 2019

Conversation

offtherailz
Copy link
Member

@offtherailz offtherailz commented Jan 9, 2019

Description

The issue for horizontal scrolling has been fixed in the latest version (see issue).
I had also to port some extension to the latest version:

  • The new version requires immutable dependency (it doesn't work without)
  • automatic scroll up on sort/filter: It was a workaround to avoid empty rows when virtual scroll is active. Now the Grid object has been changed and the old overrides that implemented this didn't work anymore. Implemented instead a forceScrollTop enhancer + scrollToTopCounter property to use to force scroll.
  • Custom RowRenderer have to use renderBaseRow to preserve ref function and so support frozen (ex - blocked) columns (tipically selection column and "zoom to feature" button in the feature grid). Otherwise the frozen columns didn't worked anymire
  • Updated tests

Issues

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix

What is the current behavior? (You can also link to an open issue here)
Grid can not be scrolled horizontally

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:
I suggest to re-do all the integration tests on the feature grid.

@coveralls
Copy link

coveralls commented Jan 9, 2019

Coverage Status

Coverage decreased (-0.3%) to 80.712% when pulling c278127 on offtherailz:fix_3405 into bc739a5 on geosolutions-it:master.

.scan((acc) => acc + 1, 0)
.map(scrollToTopCounter => ({scrollToTopCounter}))
.startWith({}),
(props, newProps) => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

I only see one Observable, why props and newProps?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are 2 observable merged into one. Maybe newProps is misleasing. Should i call it forceScrollProps?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, maybe, should make it more clear, thanks

package.json Show resolved Hide resolved
Copy link
Contributor

@mbarto mbarto left a comment

Choose a reason for hiding this comment

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

Just a couple of comments / questions, otherwise looks good

use a better name for `scrollToTopCounter` props
@offtherailz offtherailz merged commit 2db725f into geosolutions-it:master Jan 10, 2019
@offtherailz offtherailz added this to the 2019.01.00 milestone Jan 14, 2019
@offtherailz offtherailz deleted the fix_3405 branch May 26, 2020 09:39
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.

Feature grid do not work with Chrome > 70
3 participants