-
Notifications
You must be signed in to change notification settings - Fork 149
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
base: main
Are you sure you want to change the base?
Refresh indicator #190
Conversation
@ItsAdityaKSingh you can check this PR for the earlier branch was not compatible with the newest main branch changes |
Some formatting errors are failing in the test @Rushour0. Could you fix them? |
Check again |
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.
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)> |
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.
What is the exact change over here?
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.
There was a bit of formatting issue here
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.
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)> |
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.
Also here
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.
There was a bit of formatting issue here too
The single quote was causing issue, so just kinda did it out of formatting purpose
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.
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.
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.
Updates @Rushour0?
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.
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 was an extremely minor change but, felt like it would just make the .env.example look better?
It was causing issues in the release version app for some reason |
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 |
5fc16f8
to
9950443
Compare
@ItsAdityaKSingh can we merge this branch? |
Could you show me a snapshot of the error you encountered? |
This is what I was seeing in the commit previous to that on running the command |
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. |
Hey I checked the code, the issue is with the painter and will create issues on different sizes of mobile
|
@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" |
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.
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
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.
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
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.
Also, what do I do if my flutter version fails to compile it with the old package versions?
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.
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.
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 iconIssue 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
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.