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

Pull to refresh feature with UI changed #175

Closed
wants to merge 9 commits into from

Conversation

Rushour0
Copy link

@Rushour0 Rushour0 commented Nov 9, 2022

Fixes #
Added a custom pull to refresh mentioned in the issue #91
I have also made UI changes to the auth screen mentioned in the issue #158

Describe the changes you have made in this PR -
Issue 91
Made a new TabControllerStorage since we cannot late initialise the tabcontroller, and thus cannot declare it out of the build method. Storing the tab index using this singleton implementation.
Added a new package custom_refresh_indicator to implement the pull to refresh feature with a custom icon

Issue 158
Made changes to the UI to make it better looking and user friendly.

Screenshots of the changes (If any) -
Issue 91

pull_to_refresh_feature.mp4

Issue 158
login
signup

@Rushour0
Copy link
Author

Rushour0 commented Nov 9, 2022

@ItsAdityaKSingh you can check this commit, made a new branch for merging changes into it.
@nb9960 You can also check this if the need be.

Copy link
Collaborator

@ItsAdityaKSingh ItsAdityaKSingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work, @Rushour0! Overall looks good to me, though it partially closes issue #91. It still requires to be refreshed rather than automatically refreshed.

For the UI part, the buttons could be remade into a bit circular; it looks boxier than it should be on the hike and group screen. Also, for the auth screen, keeping the fields in the same card would still be preferred, honestly, though we can improvise. And the elevation also could be reduced.

lib/components/hike_button.dart Outdated Show resolved Hide resolved
lib/views/auth_screen.dart Outdated Show resolved Hide resolved
lib/views/auth_screen.dart Outdated Show resolved Hide resolved
lib/views/auth_screen.dart Outdated Show resolved Hide resolved
lib/views/group_screen.dart Outdated Show resolved Hide resolved
lib/views/group_screen.dart Outdated Show resolved Hide resolved
lib/views/group_screen.dart Outdated Show resolved Hide resolved
@Rushour0
Copy link
Author

Rushour0 commented Dec 8, 2022

Excellent work, @Rushour0! Overall looks good to me, though it partially closes issue #91. It still requires to be refreshed rather than automatically refreshed.

For the UI part, the buttons could be remade into a bit circular; it looks boxier than it should be on the hike and group screen. Also, for the auth screen, keeping the fields in the same card would still be preferred, honestly, though we can improvise. And the elevation also could be reduced.

As for the constant refresh part, I feel it will consume more data to constantly keep refreshing the data. If people are going for a hike, then the connectivity and internet might be an issue. Considering that I put a pull-to-refresh instead of a stream like constant refresher

@ItsAdityaKSingh
Copy link
Collaborator

As for the constant refresh part, I feel it will consume more data to constantly keep refreshing the data. If people are going for a hike, then the connectivity and internet might be an issue. Considering that I put a pull-to-refresh instead of a stream like constant refresher

It should not always be refreshing, but as of right now, when the timer hits 0, the whole list needs to be manually refreshed to see that change. I meant to refresh the card when it becomes active. It could be automated to refresh only the card and not the whole list when it becomes active. Are you getting the idea?

@Rushour0
Copy link
Author

As for the constant refresh part, I feel it will consume more data to constantly keep refreshing the data. If people are going for a hike, then the connectivity and internet might be an issue. Considering that I put a pull-to-refresh instead of a stream like constant refresher

It should not always be refreshing, but as of right now, when the timer hits 0, the whole list needs to be manually refreshed to see that change. I meant to refresh the card when it becomes active. It could be automated to refresh only the card and not the whole list when it becomes active. Are you getting the idea?

Ahh alright 😌, I see what you wanted to do here. Okay, so essentially instead of complete list, just refresh the card according to its timer. If this is correct then I will work on this after my academic exams are over. Will get back to you in a few weeks with the new code.

@ItsAdityaKSingh
Copy link
Collaborator

Yup, you got it right! Along with this pull-to-refresh feature, there was this to be done! And indeed, you can work on it as soon as you get over the exams :)

@Rushour0
Copy link
Author

Yup, you got it right! Along with this pull-to-refresh feature, there was this to be done! And indeed, you can work on it as soon as you get over the exams :)

Alright then, I will finish the earlier changes mentioned in the code review and submit with the refreshing feature

@ItsAdityaKSingh
Copy link
Collaborator

Any updates @Rushour0?

@Rushour0
Copy link
Author

Rushour0 commented Jan 4, 2023

Any updates @Rushour0?

Was travelling a few days and was ill for some time. Just restarted work today. Will be done in 2-3 days.

@Rushour0
Copy link
Author

Rushour0 commented Jan 5, 2023

Any updates @Rushour0?

Hey I am facing a problem
I am not able to push any updates to the rushour0 branch of my repository.
It says error: failed to push some refs

Can you help me out?

@ItsAdityaKSingh I am actually dying here, because I spent 20 minutes making changes 2 hours trying to push a commit. Somehow my repository is corrupted(?) I actually do not know what to do here.

but to show you that change you suggested of rotating refresh icon

refresh_loading

I have also made all other changes in the borders and all but I am not able to push anything.

Copy link
Author

@Rushour0 Rushour0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed elevation, border rounding, reloading icon

@Rushour0
Copy link
Author

Rushour0 commented Jan 5, 2023

Any updates @Rushour0?

Hey I am facing a problem I am not able to push any updates to the rushour0 branch of my repository. It says error: failed to push some refs

Can you help me out?

@ItsAdityaKSingh I am actually dying here, because I spent 20 minutes making changes 2 hours trying to push a commit. Somehow my repository is corrupted(?) I actually do not know what to do here.

but to show you that change you suggested of rotating refresh icon

refresh_loading

I have also made all other changes in the borders and all but I am not able to push anything.

Was still facing issue, uploaded the upgraded files manually, pulled and did a flutter test to verify

@Rushour0
Copy link
Author

Rushour0 commented Jan 8, 2023

@ItsAdityaKSingh can you review this again?

@Rushour0
Copy link
Author

@ItsAdityaKSingh hey can you check the latest changes in the PR

@ItsAdityaKSingh
Copy link
Collaborator

ItsAdityaKSingh commented Jan 22, 2023

Hey @Rushour0! Sorry I was engaged with other stuff. Will review and get this resolved soon. Appreciate the patience :)
Some tests are failing, could you fix and commit them?

@ItsAdityaKSingh
Copy link
Collaborator

Also, the issue you were facing with the commit and pull, it's fixed right now, right?

@Rushour0
Copy link
Author

Rushour0 commented Jan 22, 2023

Also, the issue you were facing with the commit and pull, it's fixed right now, right?

No. I want to smash my repository. Idk what the issue is. It just keeps denying any remote push

Copy link
Author

@Rushour0 Rushour0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed that single unnecessary variable
@ItsAdityaKSingh can you put it for checking if it fails tests

@Rushour0
Copy link
Author

Hey @ItsAdityaKSingh can you check if this can be merged?

@Rushour0
Copy link
Author

Opening a new PR for the same
This one has gotten a bit messy and unattended for a while now

@Rushour0 Rushour0 closed this Mar 10, 2023
@Rushour0 Rushour0 deleted the rushour0 branch March 10, 2023 14:09
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.

2 participants