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

Refresh indicator #190

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

Rushour0
Copy link

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) -
#91 Refresh Feature demo

pull-down-to-refresh.mp4

#158 Improved Text Form Fields
improved-text-form-field-0 improved-text-form-field-1

Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.

@Rushour0
Copy link
Author

@ItsAdityaKSingh you can check this PR for the earlier branch was not compatible with the newest main branch changes

@ItsAdityaKSingh
Copy link
Collaborator

Some formatting errors are failing in the test @Rushour0. Could you fix them?

@Rushour0
Copy link
Author

Rushour0 commented Mar 11, 2023

Some formatting errors are failing in the test @Rushour0. Could you fix them?

Check again
@ItsAdityaKSingh

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.

What changes have you made to the png? It doesn't seem to be changed.

HTTP_ENDPOINT=<servers_HTTP_endpoint>(for example: https://beacon.aadibajpai.com/graphql)>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the exact change over here?

Copy link
Author

Choose a reason for hiding this comment

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

There was a bit of formatting issue here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this would be a required change. For any formatting issue, the command you ran for the previous commit, would have edited itself.

HTTP_ENDPOINT=<servers_HTTP_endpoint>(for example: https://beacon.aadibajpai.com/graphql)>
WEBSOCKET_ENDPOINT=<servers_websocket_endpoint>(for example: wss://beacon.aadibajpai.com/subscriptions)>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also here

Copy link
Author

@Rushour0 Rushour0 Mar 11, 2023

Choose a reason for hiding this comment

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

There was a bit of formatting issue here too
The single quote was causing issue, so just kinda did it out of formatting purpose

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this be necessary? Can you show me the error without this change? As it didn't pose any problem with my app or runtime.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updates @Rushour0?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry my bad, didnt see this conversation later on

image

The formatting issue was about the .env.example having a string in it like the image attached above vs how it looks now

image

Copy link
Author

Choose a reason for hiding this comment

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

It was an extremely minor change but, felt like it would just make the .env.example look better?

@Rushour0
Copy link
Author

What changes have you made to the png? It doesn't seem to be changed.

It was causing issues in the release version app for some reason

@Rushour0
Copy link
Author

https://stackoverflow.com/questions/51220827/appmergereleaseresources-exception-when-making-release-build

Faced this kind of an issue when I was building on the new flutter version. Probably because someone converted the image from jpg to png via plainly changing the extension name from .jpg to .png, instead of actual conversion of the image.

@Rushour0
Copy link
Author

@ItsAdityaKSingh can we merge this branch?

@ItsAdityaKSingh
Copy link
Collaborator

What changes have you made to the png? It doesn't seem to be changed.

It was causing issues in the release version app for some reason

Could you show me a snapshot of the error you encountered?

@Rushour0
Copy link
Author

Rushour0 commented Mar 14, 2023

What changes have you made to the png? It doesn't seem to be changed.

It was causing issues in the release version app for some reason

Could you show me a snapshot of the error you encountered?

@ItsAdityaKSingh
image

This is what I was seeing in the commit previous to that on running the command flutter build apk --release

@ItsAdityaKSingh
Copy link
Collaborator

Just a few UI changes, could you bring down the label text of the password below to the centre of the field? Looks a bit off.
Also, the tabs in auth screen are not in the centre of the blue box around them; could you look into that too?

@Rushour0
Copy link
Author

Rushour0 commented Mar 19, 2023

Just a few UI changes, could you bring down the label text of the password below to the centre of the field? Looks a bit off. Also, the tabs in auth screen are not in the centre of the blue box around them; could you look into that too?

@ItsAdityaKSingh

Hey I checked the code, the issue is with the painter and will create issues on different sizes of mobile
So is it okay to remove the animation but keeping the design and all same

Current look Previous look
WhatsApp.Video.2023-03-19.at.14.14.30.mp4
WhatsApp.Video.2023-03-19.at.14.14.18.mp4

@Rushour0
Copy link
Author

@ItsAdityaKSingh check the last comment, what is your call on the changes?

@@ -5,254 +5,298 @@ packages:
dependency: transitive
description:
name: _fe_analyzer_shared
url: "https://pub.dartlang.org"
sha256: "4897882604d919befd350648c7f91926a9d5de99e67b455bf0917cc2362f4bb8"
Copy link
Contributor

Choose a reason for hiding this comment

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

lock file shall not be updated unless we aim to upgrade packages as it can break the app due to patch upgrades in transitive dependencies(see #203). Please remove unnecessary changes and only keep changes for the refresh indicator

Copy link
Author

Choose a reason for hiding this comment

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

Right, so I should essentially revert any lock file changes right?
Also, the hashes were added automatically, can you suggest how I should sort out this issue

Copy link
Author

Choose a reason for hiding this comment

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

Also, what do I do if my flutter version fails to compile it with the old package versions?

Copy link
Contributor

@tanisha083 tanisha083 Apr 28, 2023

Choose a reason for hiding this comment

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

regarding hashes, I think it was introduced later by dart here(dart-lang/pub#3482)
Earlier dependencies were not locked to hashes but now they're that's why you see so many autogen hash.
I think that's something we can take in some other PR, where we do 2 things, full package upgrade and make new lock file.

For now, i would suggest just using the dependency of the refresh indicator(as that's the only new package you added), and leave the rest as they're in main branch.
You can however use dart pub get --enforce-lockfile, this won't add hashes as explained here dart-lang/pub#3482 (comment)

For flutter version, which flutter are you using? Flutter 3+ works fine with the packages.

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.

3 participants