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.

Sign up for GitHub

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

Jump to bottom

Search implementation #132

Merged
merged 6 commits into from
Sep 17, 2024
Merged

Search implementation #132

merged 6 commits into from
Sep 17, 2024

Conversation

NdegwaJulius
Copy link
Contributor

@NdegwaJulius NdegwaJulius commented Sep 16, 2024 •
edited
Loading

Description

Closes #126

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

Test Changes on Device

You can download your APK from Firebase App Distribution once this PR builds successfully via the following link: https://appdistribution.firebase.dev/i/c796669942f8a811
Screenshot_20240916_190320.jpg

Screenshot_20240916_185410.jpg

Screenshot_20240916_190343.jpg

Screen_recording_20240917_100005.1.mov

@MillerAdulu
Copy link
Contributor

MillerAdulu commented Sep 16, 2024 •
edited
Loading

@NdegwaJulius ,

Please add a sample video of how this feature is working.

Sample refs:

MillerAdulu
MillerAdulu requested changes Sep 16, 2024
View reviewed changes
Copy link
Contributor

@MillerAdulu MillerAdulu left a comment

Choose a reason for hiding this comment

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

@NdegwaJulius,

Great attempt, we can iron out these minor kinks and we should be good to go once we confirm that the UX/UI is great from the video.

pubspec.yaml Outdated Show resolved Hide resolved
lib/search/cubit/search_cubit.dart Show resolved Hide resolved
lib/search/cubit/search_state.dart Outdated Show resolved Hide resolved
lib/search/models/search_result.dart Outdated Show resolved Hide resolved
lib/search/ui/search_screen.dart Outdated Show resolved Hide resolved
lib/search/ui/search_screen.dart Outdated Show resolved Hide resolved
lib/search/ui/widgets/search_bar.dart Outdated Show resolved Hide resolved
lib/search/models/search_result.dart Outdated Show resolved Hide resolved
lib/search/cubit/search_cubit.dart Outdated Show resolved Hide resolved
@MillerAdulu MillerAdulu changed the title search implementation Search implementation Sep 16, 2024
@MillerAdulu
Copy link
Contributor

@NdegwaJulius,

Please also ensure the pipeline is passing. On your local device, if your Problems tab on the Console has entries, the pipeline check won't pass hence we can't proceed to merge.

@NdegwaJulius
Copy link
Contributor Author

@NdegwaJulius ,

Please add a sample video of how this feature is working.

Sample refs:

@NdegwaJulius,

Great attempt, we can iron out these minor kinks and we should be good to go once we confirm that the UX/UI is great from the video.

Here is the Video

Screen_recording_20240917_100005.1.mov

NdegwaJulius
NdegwaJulius commented Sep 17, 2024
View reviewed changes
Copy link
Contributor Author

@NdegwaJulius NdegwaJulius left a comment

Choose a reason for hiding this comment

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

@MillerAdulu I think the issues are now fixed

NdegwaJulius
NdegwaJulius commented Sep 17, 2024
View reviewed changes
Copy link
Contributor Author

@NdegwaJulius NdegwaJulius left a comment

Choose a reason for hiding this comment

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

Fix Applied

MillerAdulu
MillerAdulu requested changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@MillerAdulu MillerAdulu left a comment

Choose a reason for hiding this comment

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

@NdegwaJulius,

Great improvements so far. Requested some minor tweaks.

lib/search/cubit/search_cubit.dart Outdated Show resolved Hide resolved
lib/search/cubit/search_cubit.dart Outdated Show resolved Hide resolved
lib/search/models/search_result.dart Outdated Show resolved Hide resolved
lib/search/ui/widgets/search_bar.dart Outdated Show resolved Hide resolved
lib/search/ui/widgets/search_bar.dart Outdated Show resolved Hide resolved
lib/bootstrap.dart Outdated Show resolved Hide resolved
lib/search/ui/widgets/search_bar.dart Outdated Show resolved Hide resolved
lib/search/models/search_result.dart Outdated Show resolved Hide resolved
lib/search/models/search_result.dart Outdated Show resolved Hide resolved
lib/search/ui/widgets/search_bar.dart Outdated Show resolved Hide resolved
NdegwaJulius
NdegwaJulius commented Sep 17, 2024
View reviewed changes
Copy link
Contributor Author

@NdegwaJulius NdegwaJulius left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-09-17 at 20 22 16

MillerAdulu
MillerAdulu requested changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@MillerAdulu MillerAdulu left a comment

Choose a reason for hiding this comment

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

Almost there. Minor tweaks that have come up, should be a few more minutes of your time. Thank you for taking the time to apply these corrections.

lib/bootstrap.dart Outdated Show resolved Hide resolved
lib/search/ui/widgets/search_bar.dart Outdated Show resolved Hide resolved
lib/search/cubit/search_cubit.dart Show resolved Hide resolved
lib/common/repository/db_repository.dart Outdated Show resolved Hide resolved
@MillerAdulu
Copy link
Contributor

@NdegwaJulius,

A code formatting concern, do run the fmt: command in the Makefile to fix this.

@MillerAdulu
Copy link
Contributor

@NdegwaJulius,

Please also update 1.16.01+11601 to 1.17.00+11700

NdegwaJulius
NdegwaJulius commented Sep 17, 2024
View reviewed changes
Copy link
Contributor Author

@NdegwaJulius NdegwaJulius left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-09-17 at 23 05 29

@NdegwaJulius
Copy link
Contributor Author

@NdegwaJulius,

Please also update 1.16.01+11601 to 1.17.00+11700

Updated

@MillerAdulu
Copy link
Contributor

Thanks @NdegwaJulius for sweating this out. Lovely work.

@MillerAdulu MillerAdulu added this pull request to the merge queue Sep 17, 2024
Merged via the queue into droidconKE:main with commit 956a4f6 Sep 17, 2024
1 check passed
@NdegwaJulius
Copy link
Contributor Author

NdegwaJulius commented Sep 17, 2024 •
edited
Loading

It was a great one actually, @MillerAdulu thanks for taking your time reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MillerAdulu MillerAdulu MillerAdulu approved these changes

Assignees

@MillerAdulu MillerAdulu

@mulieriq mulieriq

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Homepage Search
3 participants
@NdegwaJulius @MillerAdulu @mulieriq