Skip to content

Refactor search functionality#302

Merged
joshuadeguzman merged 6 commits into
flutterph:masterfrom
OliverRhyme:search
Feb 23, 2021
Merged

Refactor search functionality#302
joshuadeguzman merged 6 commits into
flutterph:masterfrom
OliverRhyme:search

Conversation

@OliverRhyme

@OliverRhyme OliverRhyme commented Feb 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

@joshuadeguzman
Refactors the search functionality with the flexibility to easily integrate Jobs search.
Edit: Add Developer loading

OliverRhyme
OliverRhyme commented Feb 15, 2021
View reviewed changes
Comment thread lib/features/devboard/devboard_page.dart Outdated
@joshuadeguzman joshuadeguzman added this to the Devboard 🏆 milestone Feb 15, 2021
@joshuadeguzman

joshuadeguzman commented Feb 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Hello @OliverRhyme,

Few things from me,

  • Please add the screenshots for the changes made (preferably GIF or video?)
  • Create issue tickets for the changes you made, eg. create an issue for refactoring search, then reference this PR to that issue
  • Please rebase to the latest commit

Thanks for this.

@joshuadeguzman
joshuadeguzman self-requested a review February 15, 2021 02:28
@OliverRhyme

OliverRhyme commented Feb 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hello @OliverRhyme,

Few things from me,

  • Please add the screenshots for the changes made (preferably GIF or video?)
  • Create issue tickets for the changes you made, eg. create an issue for refactoring search
  • Please rebase to the latest commit

Thanks for this.

I am able to run the page on my github fork at https://oliverrhyme.github.io/devs/

joshuadeguzman
joshuadeguzman requested changes Feb 15, 2021
View reviewed changes

@joshuadeguzman joshuadeguzman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hello @OliverRhyme,

Thanks for the PR. 🎉

Kindly review the comments. For the meantime, let's ignore issues that are not related to the PR, eg. providing repository and data source, to keep the PR small as possible.

cc: @Ram231 @Jansalvador1445

Comment thread .all-contributorsrc
Comment thread lib/core/widgets/components/search_bar.dart Outdated
Comment thread lib/features/dashboard/dashboard_model.dart
Comment thread lib/features/dashboard/dashboard_model.dart
Comment thread lib/features/dashboard/dashboard_page.dart Outdated
Comment thread lib/features/devboard/devboard_page.dart Outdated
Comment thread lib/features/devboard/devboard_page.dart Outdated
Comment thread lib/main.dart
This was referenced Feb 15, 2021
Open
Open
@OliverRhyme

OliverRhyme commented Feb 15, 2021

Copy link
Copy Markdown
Contributor Author

Hello @OliverRhyme,

Thanks for the PR. 🎉

Kindly review the comments. For the meantime, let's ignore issues that are not related to the PR, eg. providing repository and data source, to keep the PR small as possible.

cc: @Ram231 @Jansalvador1445

I've just refactored, as it is needed for the search to work properly

This was referenced Feb 15, 2021
Open
Open
@OliverRhyme
OliverRhyme force-pushed the search branch 3 times, most recently from cd4919f to f87b877 Compare February 15, 2021 04:35
@OliverRhyme

OliverRhyme commented Feb 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Separated dev loading indicator into another pull request. Will request another pull request when this is merged to avoid conflict.

@OliverRhyme

OliverRhyme commented Feb 16, 2021

Copy link
Copy Markdown
Contributor Author

Hi po @joshuadeguzman @Jansalvador1445 pa review po. I've already addressed some of the concerns.

Note: This will overwrite the #282 since its not compatible, but I have my own implementation with the follow up PR which can easily be improved.

vinceramcesoliveros
vinceramcesoliveros reviewed Feb 16, 2021
View reviewed changes
Comment thread lib/utils/utils.dart
vinceramcesoliveros
vinceramcesoliveros approved these changes Feb 16, 2021
View reviewed changes

@vinceramcesoliveros vinceramcesoliveros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@joshuadeguzman
joshuadeguzman merged commit 3051f65 into flutterph:master Feb 23, 2021
@joshuadeguzman

joshuadeguzman commented Feb 23, 2021

Copy link
Copy Markdown
Member

@all-contributors please add @OliverRhyme for code, bug

@allcontributors allcontributors Bot mentioned this pull request Feb 23, 2021
Merged
@allcontributors

allcontributors Bot commented Feb 23, 2021

Copy link
Copy Markdown
Contributor

@joshuadeguzman

I've put up a pull request to add @OliverRhyme! 🎉

@OliverRhyme OliverRhyme mentioned this pull request Feb 23, 2021
Open
@OliverRhyme
OliverRhyme deleted the search branch February 23, 2021 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vinceramcesoliveros vinceramcesoliveros vinceramcesoliveros approved these changes
@joshuadeguzman joshuadeguzman joshuadeguzman approved these changes
@Jansalvador1445 Jansalvador1445 Awaiting requested review from Jansalvador1445

Assignees

@OliverRhyme OliverRhyme

Labels

3 points #30daysOfFlutter

Projects

None yet

Milestone

Devboard 🏆

Development

Successfully merging this pull request may close these issues.

3 participants

@OliverRhyme @joshuadeguzman @vinceramcesoliveros