Added paging support to scrollview and ART #603
Conversation
|
Not sure how the new base should align with existing implementation such that everything remains composable. Help out, I have to add scrollTo logic |
| @@ -65,7 +65,8 @@ | |||
| "inline-style-prefixer": "^3.0.6", | |||
| "normalize-css-color": "^1.0.2", | |||
| "prop-types": "^15.5.10", | |||
| "react-timer-mixin": "^0.13.3" | |||
| "react-timer-mixin": "^0.13.3", | |||
| "@ecliptic/react-art": "0.15.4-fork.1" | |||
There was a problem hiding this comment.
Why the this fork instead of the one owned by FB?
There was a problem hiding this comment.
Check out this issue reactjs/react-art#113
The npm package no longer worked with latest react version, unfortunately no one is publishing it :(
There was a problem hiding this comment.
I'll consider this once the official package can be used
There was a problem hiding this comment.
Why don't you fork and publish one yourself? We can use that
There was a problem hiding this comment.
No thanks. Let's get the official package updated
| @@ -1,2 +1,3 @@ | |||
| node_modules | |||
| dist | |||
| .idea | |||
| timeStamp: Date.now() | ||
| }); | ||
|
|
||
| export default class PagingScrollViewBase extends Component { |
There was a problem hiding this comment.
Because it does not really use the base at all. Scroll events are raised using onTouchMove, you can snap browser scrolling. Switching the base seemed like a good idea to me. I'm open to ideas here since you know the code better
There was a problem hiding this comment.
What about scrolling without touch?
| @@ -39,6 +39,7 @@ class View extends Component { | |||
| const { | |||
| hitSlop, | |||
| style, | |||
| domRef, | |||
There was a problem hiding this comment.
I had wrtiten a comment around this, onLayout does not fire and measure doesn't work on one of the views that I created. Needed dom node for clientWidth, let me know the alternative
There was a problem hiding this comment.
Didn't have the time to look into the missing callbacks issue
There was a problem hiding this comment.
OK. I don't think I will accept a patch that hacks around what is apparently another issue though
|
Also, please break this into 2 PRs. One for ART and the other for paging. Thanks |
|
I know there should be multiple PRs. I'm mostly trying to understand contributing to this project right now. Just reply to my question, we can close this PR post that. I'll raise two in the future. |
Which question is that? |
|
I meant respond to my replies to your review comments. |
|
I think you can break this into 2 PRs now |
|
Closing this PR for now. ART was added in 321051b. |
Addresses #601 , #602