-
Notifications
You must be signed in to change notification settings - Fork 984
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_: reloading of contacts when selected and unselected #20582
Conversation
Jenkins BuildsClick to see older builds (37)
|
hi linked issue is probably wrong #20513 |
updated it thanks @Parveshdhull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't look like a proper fix, because the gesture is used for the purpose here, we need to find the reason and fix it, we should keep the gesture
795fda6
to
efd8a5a
Compare
@@ -50,7 +50,7 @@ | |||
item]))))) | |||
|
|||
(defn add-manage-members | |||
[{:keys [scroll-enabled? on-scroll]}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we can't just remove these props? they are needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flexsurfer do you know why we need them. I dont see why we need them on this part and scroll-enabled?
is the cause for this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is used to close the screen by scrolling, but still we need to know the reason, we can't just remove functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flexsurfer the screen here will always re-render when scroll-enabled
is updated and this is why scrolling stops working for a moment because its actually disabled.
I am not sure but assuming this is reason why this enabling and disabling of scroll was avoided in other areas like invite contacts to community
where its the same list of contacts and the same functionality would probably be needed
7dc5277
to
efd8a5a
Compare
8c49de3
to
b68f387
Compare
@flexsurfer @ilmotta I think we can do without the functionality of scroll enabled, just like in another parts of the app. When the scolling is not disabled the screen will not reload and the state of the selected contacts is saved on-scroll. I found enabling and disabling scroll abit confusing for the user, the first instance the list scrolls the second the scroll is disabled and the modal screen would then be closed, the third, the list would scroll and fourth scroll is disabled. I believe this is not the behaviour the user wants and its a bit inconsistent with other places where we are scrolling contacts of even just list items. Most times the user would close the modal screen when they actually intend to just scroll the screen. I suggest that we merge the pr as is which basically remove enabling and disabling the scroll function for the user to close the screen. |
b68f387
to
3ab44d7
Compare
It becomes a problem of the lesser of two evils @jo-mut @flexsurfer. I think the bug that resets the selection of contacts is significantly worse than the bug we are now introducing in this PR, thus I think your suggestion to move forward without the bottom sheet scroll behavior is okay. I'm pretty sure there's a proper solution that keeps the scroll behavior and fix the contact selection, but given our time constraints, fixing contact selection is more important to users in 2.30. |
When I scroll down, the contact list reloads and becomes unresponsive for a moment. Is this expected? What exactly happens during that reload? |
@ajayesivan, from what I tested a few days ago, there seems to be a fundamental problem with the way the code is handling the local state and/or subscription that causes the entire thing to re-render and lose the previous state. This would explain why things appear to refresh. Do you think this problem is better than the original problem we are solving in the PR? If so, then I think we can merge this PR right away and improve for the next release. |
… add to a group chat
3ab44d7
to
74c3818
Compare
@ilmotta This is better than the original problem. I suggest we merge this and create a follow-up issue to tackle the re-render problem. |
fixes #20532
Summary
When the user navigates to the contact screen, the contact list item is already pre-checked for the group members. The user then has the option to either add new contacts to the group or the group members.
The contact list item initialises an
atom
that gets updated when the user selects and unselects a contact however when the screen reloads and the atom loses state to the default value and because of this, the user has to repeat the process again