Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

feat: add loading indicator when changing password and saving profile #83

Merged

Conversation

techno-disaster
Copy link
Contributor

@techno-disaster techno-disaster commented May 21, 2020

Description

Add loading indicator on screen to help user understand what's happening.
If a user has a slow internet connection, the changing password request can take long and without a loading indicator he/she might think the apps stuck. Loading indicators will tell that the process is currently going on
Fixes #67
Fixes #94

Flutter Channel:

  • I have used the Flutter Beta channel on my local machine

Type of Change:

Delete irrelevant options.

  • User Interface

Code/Quality Assurance Only

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Physical device. Check gif below
ezgif com-video-to-gif

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas

Code/Quality Assurance Only

  • My changes generate no new warnings

@techno-disaster
Copy link
Contributor Author

@anitab-org/qa-team can someone test this?
Steps :
Change password, a loading indicator like in the gif should load up, until the snackbar comes up
Change something in Profile and save profile, a loading indicator should show up until the snacbar comes

lib/screens/home/pages/profile/profile_page.dart Outdated Show resolved Hide resolved
lib/screens/settings/settings_screen.dart Outdated Show resolved Hide resolved
@bartekpacia
Copy link
Contributor

Great job overall👍

bartekpacia
bartekpacia previously approved these changes May 22, 2020
Copy link
Contributor

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

Looks good.
EDIT: Looks I had a second reviewing screen opened in the second browser tab🤦‍♀️

@techno-disaster
Copy link
Contributor Author

Looks good.

Still have to make some changes.

@techno-disaster techno-disaster added the Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. label May 24, 2020
@techno-disaster
Copy link
Contributor Author

Found one more page which can use loader and cleaned he code a bit

ezgif com-video-to-gif(1)

@techno-disaster
Copy link
Contributor Author

I had to remove final from the bloc because I wanted to set the message to null, after we show the snackbar. without this the loader runs fine for the first time, but if the user hits send request again, then as state.message is not null anymore it wont enter the ifcase, and no snackbar will be shown + continuous loading.

bartekpacia
bartekpacia previously approved these changes May 28, 2020
Copy link
Contributor

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

LGTM

@techno-disaster
Copy link
Contributor Author

@isabelcosta can you review this too? 👀 Thanks

@rpattath rpattath added Status: Needs Review PR needs an additional review or a maintainer's review. and removed Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. labels Jun 2, 2020
robotjellyzone
robotjellyzone previously approved these changes Jun 8, 2020
Copy link

@robotjellyzone robotjellyzone left a comment

Choose a reason for hiding this comment

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

Looks good to me @techno-disaster ! just want to know why did you downgraded some of the versions in pubspec file ?

@techno-disaster
Copy link
Contributor Author

Looks good to me @techno-disaster ! just want to know why did you downgraded some of the versions in pubspec file ?

The pubspec.lock is an auto generated file. We store all our dependencies in pubspec.yaml file.

@techno-disaster techno-disaster added the Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. label Jun 11, 2020
@techno-disaster
Copy link
Contributor Author

Working on #94 (It needs loading indicators as well). Lets review it after that's pushed too

HaripriyaB
HaripriyaB previously approved these changes Jun 12, 2020
bartekpacia
bartekpacia previously approved these changes Jun 12, 2020
Copy link
Contributor

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

nice!

@robotjellyzone
Copy link

@anitab-org/qa-team can someone give this a quick test?
Thank You

I have tested this PR @techno-disaster as per your request . The needed feature is working as can be seen below :-
testing

@robotjellyzone
Copy link

But I don't know why relation page isn't working as it can be seen from the following gif that all other pages are working but i am unable to see the relation page as its causing the app to crash ... with the following message on logcat -->

INFO: 2020-06-13 16:38:29.914830: Transition { currentState: RelationPageLoading, event: RelationPageShowed, nextState: RelationPageFailure }

relation-page-not-working

Copy link

@robotjellyzone robotjellyzone left a comment

Choose a reason for hiding this comment

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

May be the changes done in the relation page needs to be reviewed again 🤔 @techno-disaster ?

@techno-disaster
Copy link
Contributor Author

@robotjellyzone thanks for reporting this, this happened when the user was not in a relationship, the issue is now fixed. Can you test again? i also added a loading indicator to the signUp page.

@techno-disaster
Copy link
Contributor Author

@anitab-org/qa-team can anyone give this a quick test again? Thank You

robotjellyzone
robotjellyzone previously approved these changes Jun 16, 2020
Copy link

@robotjellyzone robotjellyzone left a comment

Choose a reason for hiding this comment

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

Hi @techno-disaster i have tested the required feature & the relation page , everything is now working fine

great 👍

testing_pr_2

bartekpacia
bartekpacia previously approved these changes Jun 16, 2020
Copy link
Contributor

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

Nice job :)

lib/widgets/loading_indicator.dart Show resolved Hide resolved
@techno-disaster
Copy link
Contributor Author

@anitab-org/mentorship-flutter-maintainers Can you take a quick look at this? The PR has been tested (all OK) just needs a quick review now

Copy link
Member

@isabelcosta isabelcosta left a comment

Choose a reason for hiding this comment

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

Knowing very little about flutter, I am approving because this functionality seems to work.
@techno-disaster great work.
I am counting on other reviewers for code quality check. @bartekpacia you have to re-review after recent code changes.

@techno-disaster
Copy link
Contributor Author

@bartekpacia could you check this again? previous review git dismissed due to the topcontext > topContext change

Copy link
Contributor

@bartekpacia bartekpacia left a comment

Choose a reason for hiding this comment

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

thanks for using the camelCase, good job!

@techno-disaster techno-disaster merged commit e97fd3d into anitab-org:develop Jun 22, 2020
@techno-disaster techno-disaster deleted the issue-67-loading-indicators branch June 22, 2020 09:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Loading indicator when a task is created and marked as complete Add loading indicators
7 participants