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

@OliverRhymeOliverRhyme 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 threadlib/features/devboard/devboard_page.dart Outdated
@joshuadeguzmanjoshuadeguzman 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
ContributorAuthor

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

@joshuadeguzmanjoshuadeguzman 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 threadlib/core/widgets/components/search_bar.dart Outdated
Comment threadlib/features/dashboard/dashboard_model.dart
Comment threadlib/features/dashboard/dashboard_model.dart
Comment threadlib/features/dashboard/dashboard_page.dart Outdated
Comment threadlib/features/devboard/devboard_page.dart Outdated
Comment threadlib/features/devboard/devboard_page.dart Outdated
Comment threadlib/main.dart
This was referenced Feb 15, 2021
Open
Open
@OliverRhyme

OliverRhyme commented Feb 15, 2021

Copy link
Copy Markdown
ContributorAuthor

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
OliverRhymeforce-pushed the search branch 3 times, most recently from cd4919f to f87b877CompareFebruary 15, 2021 04:35
@OliverRhyme

OliverRhyme commented Feb 15, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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
ContributorAuthor

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 threadlib/utils/utils.dart
vinceramcesoliveros
vinceramcesoliveros approved these changes Feb 16, 2021
View reviewed changes

@vinceramcesoliverosvinceramcesoliveros 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:masterFeb 23, 2021
@joshuadeguzman

joshuadeguzman commented Feb 23, 2021

Copy link
Copy Markdown
Member

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

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

allcontributorsBot commented Feb 23, 2021

Copy link
Copy Markdown
Contributor

@joshuadeguzman

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

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

Reviewers

@vinceramcesoliverosvinceramcesoliverosvinceramcesoliveros approved these changes
@joshuadeguzmanjoshuadeguzmanjoshuadeguzman approved these changes
@Jansalvador1445Jansalvador1445Awaiting requested review from Jansalvador1445

Assignees

@OliverRhymeOliverRhyme

Labels

3 points#30daysOfFlutter

Projects

None yet

Milestone

Devboard 🏆

Development

Successfully merging this pull request may close these issues.

3 participants

@OliverRhyme@joshuadeguzman@vinceramcesoliveros