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

Proper State Management (Removal of pull to refresh ) #334

Closed
ishan0805 opened this issue Mar 15, 2021 · 15 comments
Closed

Proper State Management (Removal of pull to refresh ) #334

ishan0805 opened this issue Mar 15, 2021 · 15 comments
Assignees
Labels
bug Something isn't working

Comments

@ishan0805
Copy link
Contributor

Description

I think pull to refresh can be avoided by proper state management . As of now manually a request is made again and again to fetch data when page is reloaded .

Solution

Inside preference provider we can define a function that return the list of post with a 'listener()' so that whenever a new post is created that is displayed automatically without explicitly pulling the page to refresh .
OR
It can also be solved by using Streams , using BehaviorSubject Observable provided by Redux , witch is a better way of doing it as it sends the very last event that was emitted to the listener that just subscribed but as the app is build mostly with provider it will not match the MVVM design .

Benefits

  1. By following this approach multiple calling of setstate() can be avoided . As the number of news increase it will be more optimal .
  2. As list of news will be provided by preference Provided , it will be globally available for every widget below root.
  3. It will remove the logic part from the UI part hence improve testing and become more readable .

Screenshot_2021-03-15-22-50-47-684_com example talawa

@yasharth291
Copy link
Contributor

Hello @ishan0805 actually i think you are not working on the recent code because the thing i can see on the screenshot is old and can create problem with my merge please see mentors

@yasharth291
Copy link
Contributor

And i think it is resolved in the current code base

@ishan0805
Copy link
Contributor Author

ok I will check that , thanks for the information .

@ishan0805
Copy link
Contributor Author

ishan0805 commented Mar 16, 2021

@yasharth291 only the pull to remove text is removed the functionally is still the same . Now user need to pull himself without knowing if its news is posted or not . Don't worry I saw you have made changes to the UI and I am talking about flow of data , it will not affect your code

WhatsApp.Video.2021-03-16.at.11.14.16.AM.mp4

@palisadoes palisadoes added the unapproved Unapproved, needs to be triaged label Mar 16, 2021
@jordanliu jordanliu removed the unapproved Unapproved, needs to be triaged label Mar 17, 2021
@Sagar2366
Copy link
Contributor

@ishan0805 you can start working on this issue. Consider this case also while working on this issue - #300

@Sagar2366
Copy link
Contributor

@piyushpradhan as discussed in slack channel please start working on this issue.

@Sagar2366 Sagar2366 added bug Something isn't working help wanted and removed enhancement labels Mar 21, 2021
@piyushpradhan
Copy link

Yes, I'm on it.

@Sagar2366
Copy link
Contributor

@piyushpradhan because of no activity on this issue, we will have to un-assign this issue and let someone else work on it.

@Sagar2366 Sagar2366 assigned ishan0805 and unassigned piyushpradhan Mar 25, 2021
@StrangeNoob
Copy link
Contributor

@Sagar2366 I am working on it with @piyushpradhan. Can you assign this to me ?

@Sagar2366
Copy link
Contributor

@Sagar2366 I am working on it with @piyushpradhan. Can you assign this to me ?

@StrangeNoob any progress on this issue ?
Can you please raise PR and add issue number to it.

@ishan0805 wait for some time, do not start working on this issue.

@StrangeNoob
Copy link
Contributor

@Sagar2366 I started working on it yesterday. I will let you know by tomorrow. Is it Okay?

@Sagar2366
Copy link
Contributor

@StrangeNoob did we discussed about you would be working on this issue in slack ?
If yes raise PR With whatever work is done.

@StrangeNoob
Copy link
Contributor

closes #334 via #518

@CyberWake
Copy link
Contributor

@ishan0805 we have implemented the mvvm in the application and we are further optimising the code base this issue will be resolved uder #850 . may we close this issue?

@CyberWake
Copy link
Contributor

@palisadoes sir we can close this issue as #850 will handle this one and resolve the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

8 participants