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

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

Merged
merged 1 commit into from
Jul 5, 2019

Conversation

n-sviridenko
Copy link
Contributor

@n-sviridenko n-sviridenko commented Jun 26, 2019

Solves #114

@coveralls
Copy link

coveralls commented Jun 26, 2019

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%
Totals Coverage Status
Change from base Build 424: -0.3%
Covered Lines: 1207
Relevant Lines: 1420

💛 - Coveralls

@n-sviridenko
Copy link
Contributor Author

n-sviridenko commented Jun 29, 2019

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

@andon
Copy link
Member

andon commented Jul 1, 2019

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
Copy link
Collaborator

bevkoski commented Jul 3, 2019

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:

The following to elementSize:

And the following to isViewportOffsetBeforeElement:

if (isViewportOffsetAboveElement) {

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

@andon andon requested a review from bevkoski July 4, 2019 07:49
@bevkoski bevkoski changed the title Support horizontal scrolling feat(components): Extend viewport awareness for horizontally scrollable views Jul 5, 2019
@bevkoski
Copy link
Collaborator

bevkoski commented Jul 5, 2019

The suggested refactoring was done in a separate PR: #139. This one is ready for merging.

@bevkoski bevkoski merged commit c8adda1 into netceteragroup:master Jul 5, 2019
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.

4 participants