feat(components): Extend viewport awareness for horizontally scrollable views

netceteragroup / skele

Architectural framework that assists building data-driven apps with React or React Native.

MIT License

163 stars 32 forks source link

feat(components): Extend viewport awareness for horizontally scrollable views #138

Closed n-sviridenko closed 5 years ago

n-sviridenko commented 5 years ago

Solves https://github.com/netceteragroup/skele/issues/114

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 438

6 of 12 (50.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.3%) to 81.337%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/components/src/viewport/aware/index.js	0	6	0.0%
<!--	Total:	6	12	50.0%	-->

Totals
Change from base Build 424:	-0.3%
Covered Lines:	1207
Relevant Lines:	1420

💛 - Coveralls

n-sviridenko commented 5 years ago

Are there any updates on it? @ognen @andon @bevkoski

andon commented 5 years ago

Hi @n-sviridenko,

Thanks a lot for the efforts to implement this. To me it looks fine, but @bevkoski is the expert for this and he's been enjoying a vacation these days. He knows the nitty-gritty details of it, and I would like him to test it in our main app before it's being merged.

If you are extremely dependent on it (i.e. for some reason can't depend on your branch), I can merge it and cut a release.

bevkoski commented 5 years ago

Great job @n-sviridenko! 👏 I could quickly confirm that your code works using the following Snack: https://snack.expo.io/Bkl6Y75xB Let's do several very small improvements and release this.

It makes sense to rename the following parameter to viewportSize: https://github.com/netceteragroup/skele/blob/b7758efa95202938d2d40135434e67c93da05d45/packages/components/src/viewport/utils.js#L5

The following to elementSize: https://github.com/netceteragroup/skele/blob/b7758efa95202938d2d40135434e67c93da05d45/packages/components/src/viewport/utils.js#L7

And the following to isViewportOffsetBeforeElement: https://github.com/netceteragroup/skele/blob/b7758efa95202938d2d40135434e67c93da05d45/packages/components/src/viewport/utils.js#L17

Since we are using the same function for calculating both inVerticalViewport and inHorizontalViewport.

bevkoski commented 5 years ago

The suggested refactoring was done in a separate PR: https://github.com/netceteragroup/skele/pull/139. This one is ready for merging.