-
Notifications
You must be signed in to change notification settings - Fork 273
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
feature: UserEvent scroll()
#1441
Comments
CC: @pierrezimmermannbam @MattAgn @vanGalilea perhaps any of you would be interested? :-) |
Hi, do you mind if I tried? |
@siepra go ahead. In order to achieve the high-quality simulation pls submit experiments app PR first, then fill in the Wiki, only then start to work on the actual code (as described in the issue desc). |
Linking a PR (draft) for experiments #1442 |
You should log it in experiment, but probably does not happen on actual scrolling |
@siepra step 1 merged. Time for recording logged events on wiki. |
Great, thank you! |
@siepra I've copied your research into the wiki. Good work! Now we need to decide on the API for user interactions:
Some initial thoughts about API:
These are just rough ideas, feel free to challenge them, you can also start coding the implementations & tests to get a better feel for the API. |
I think you're right with most of those ideas. Nevertheless setting x and y values may be frustrating if one doesn't know the exact content size of the scrollable element so I'm wondering if it might be better to let the user type in e.g. percentage value? I'm not really sure what the real life use-case here is. I think the extremes will be the most useful: Also I'm curious what the number of |
RNTL will not be able to calculate the exact content size, so we should allow user to somehow specify it, e.g. by passing re. scrollToTop is a special action invoked in native iOS then you tap the status bar and it scrolls the scroll view all the way to the top. it has it's own event handler. This is different then user just scrolling all the way to the top. There is no re number of |
Bummer, I thought we're able to calculate content size and make it smarter. I'm worried about validating host element type and writing tests for
|
You should pass a function creating actual JSX elements to |
You mean like:
It doesn't work neither |
@siepra You're right, I was able to replicate it easily. tbh I was not aware of that, so that's for catching it. Anyway, let's work on |
So far I've come with
A They both work with There are some questions that popped into my head:
Remaining things:
Is there something else I'm missing? |
re saving values: The way we could achieve this is to have some
Additionally, iirc. We need to confirm experimentally that it works, but it should work fine :-) |
If understand correctly the API the user wouldn't need to pass an element to the scroll function. I think it's a good idea but in that case how do we know on which element to apply the scroll events? |
@pierrezimmermannbam they definitely need to pass element there. Otherwise how would we know which element to scroll? |
@mdjastrzebski it works :) TypeScript doesn't like the idea of Also, in tests the rendered |
Got it! |
@mdjastrzebski I think the only remaining thing is |
(FlatList) not sure if it's helpful but I see the problem is re-creating component instance at I haven't been lurking that much into rendering components yet, so I probably need to learn more about an overall context but maybe it'll save someone some logs printing |
I've created a separate issue for FlatList in #1449, Edit: this seems to be issue occurring in our repo, as |
Well usually there is a single element scrollable on a screen. There may be more but they are most of the time not in the same direction. For most use cases we could detect either a scrollview or a flat list and use that I think. Maybe the element could be optional so that in case user has nested scroll views he may be able to target one specifically? Because otherwise how do you target a scrollview if not with a test id? |
I think we should require element for now, as it matches the convention of all other FireEvent/UserEvent API which also require element to trigger event/interaction on. Users are familiar with this pattern, and it makes the tests more robust, in cases user introduces another scroll view in the complex component. There are valid UI patterns when you have 2+ scroll views, e.g. 1+ horizontal scroll views that can be scrolled independently inside overall vertical scroll view. |
In the case where there is an horizontal and a vertical scrollview we would know which one to scroll on based on the direction of the scroll but it wouldn't work if there are several horizontal scroll views though. Some other cases like scrollviews within tabs may also be problematic so maybe it's not a good idea after all. I thought about this because in e2e tests you don't provide the element but unfortunately we have limitations that prevent us from having the same API so I agree it's better to have the element being mandatory |
I've created a separate issue for discussion just the |
Resolved in #1445 |
Describe the Feature
Create a User Event interaction(s) for scrolling. This seems to be pretty frequent interaction on touch based devices, at the same time there are numerous events involved:
onScroll
,onMomentumScrollBegin/End
, onScrollBegin/EndDrag, etc. This feature should work with
ScrollViewas well as
FlatList/
SectionList` (hopefully these will be same event sequences).Possible Implementations
As with other User Event interactions:
Related Issues
#1405 - core User Event code
#1386 -
press()
implementation#1396 -
type()
implementationThe text was updated successfully, but these errors were encountered: