Skip to content

feat(ui_firestore): implemented separators and fetching indicator #99

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

Merged
merged 12 commits into from
Jan 4, 2024

Conversation

ShahoodulHassan
Copy link
Contributor

@ShahoodulHassan ShahoodulHassan commented Sep 5, 2023

Added the following features to FirestoreListView:

  • show separators between list items
  • show fetching indicator when the list reaches the end
  • show both fetching indicator and separators

Description

Replace this paragraph with a description of what this PR is doing. If you're modifying existing behavior, describe the existing behavior, how this PR is changing it, and what motivated the change.

Related Issues

Replace this paragraph with a list of issues related to this PR from the issue database. Indicate, which of these issues are resolved or fixed by this PR.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • All unit tests pass (melos run test:unit:all doesn't fail).
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

- show separators between list items
- show loading indicator when the list reaches the end
- show both loading indicator and separators
@google-cla
Copy link

google-cla bot commented Sep 5, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@ShahoodulHassan ShahoodulHassan changed the title Implemented separators and loading indicator feat: implemented separators and loading indicator Sep 5, 2023
@lesnitsky
Copy link
Member

format action is failing because new files are generated during melos bootstrap, this was fixed on the main branch, please rebase the latest main to get format green.

@ShahoodulHassan ShahoodulHassan force-pushed the separated_loading_indicator branch from 1e975bc to 836dc31 Compare October 3, 2023 18:15
@lesnitsky lesnitsky changed the title feat: implemented separators and loading indicator feat(ui_firestore): implemented separators and loading indicator Oct 5, 2023
@ShahoodulHassan
Copy link
Contributor Author

format action is failing because new files are generated during melos bootstrap, this was fixed on the main branch, please rebase the latest main to get format green.

As a first step, I tried to update the main branch in my fork (using Sync Fork) but I ended up with many errors that I was unable to fix. Is there any other way to achieve this?

@lesnitsky
Copy link
Member

  1. add an origin to your local git checkout: git remote add upstream git@github.com:firebase/FirebaseUI-Flutter.git
  2. git checkout main
  3. git pull upstream main (syncs the latest main branch of this repo with your fork)
  4. git checkout <your-branch>
  5. git merge main
  6. resolve merge conflicts, if any
  7. commit and push

@ShahoodulHassan
Copy link
Contributor Author

  1. add an origin to your local git checkout: git remote add upstream git@github.com:firebase/FirebaseUI-Flutter.git
  2. git checkout main
  3. git pull upstream main (syncs the latest main branch of this repo with your fork)
  4. git checkout <your-branch>
  5. git merge main
  6. resolve merge conflicts, if any
  7. commit and push

After performing step 3, I get to the same stage where I was using 'Sync fork'.
There are errors in packages/firebase_ui_auth and tests/integration_test that I can't happen to fix.
Please let me know how to work around it.

@lesnitsky
Copy link
Member

There are errors in packages/firebase_ui_auth and tests/integration_test that I can't happen to fix.

What errors exactly are you getting?

@lesnitsky
Copy link
Member

@ShahoodulHassan will you be able to address PR review feedback?

@ShahoodulHassan
Copy link
Contributor Author

Sure I'll provide the feedback on errors by tomorrow max.
Thanks for your patience!

…dicator have been removed.

This functionality is now achieved via constructor properties.
@ShahoodulHassan
Copy link
Contributor Author

There are errors in packages/firebase_ui_auth and tests/integration_test that I can't happen to fix.

What errors exactly are you getting?

I checked again today and the errors are all gone.

…ngIndicator anymore.

2) showLoadingIndicator is renamed to showFetchingIndicator. FirestoreLoadingIndicatorBuilder is also renamed to FirestoreFetchingIndicatorBuilder accordingly.
3) LoadingIndicator has been used from firebase_ui_shared package
4) Unnecessary comments deleted.
5) BuildAwareWidget is now called OnMountListener and its onBuild method is called onMount.
6) onMount can be called with a delay, that can be configured as per requirement. A default delay of 500ms has been used here to make the fetching indicator show for a bit of a time.
@ShahoodulHassan ShahoodulHassan changed the title feat(ui_firestore): implemented separators and loading indicator feat(ui_firestore): implemented separators and fetching indicator Nov 25, 2023
@ShahoodulHassan
Copy link
Contributor Author

@lesnitsky I've incorporated all requested changes via this commit.
Kindly review these and provide your valuable feedback!

Copy link
Member

@lesnitsky lesnitsky left a comment

Choose a reason for hiding this comment

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

@lesnitsky
Copy link
Member

Please resolve conflicts

@ShahoodulHassan
Copy link
Contributor Author

Please resolve conflicts

Done!

@ShahoodulHassan
Copy link
Contributor Author

See this comment

I've submitted reply to this comment and also resolved the conflicts. Now waiting for your further feedback plz....!

@ShahoodulHassan
Copy link
Contributor Author

ShahoodulHassan commented Nov 30, 2023

@lesnitsky What should I do in order to pass all the failing checks?

@lesnitsky
Copy link
Member

@ShahoodulHassan I'm taking care of those, no action required from your side.

@ShahoodulHassan
Copy link
Contributor Author

@ShahoodulHassan I'm taking care of those, no action required from your side.

Is it gonna take more time for merging this into the main branch?

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