Skip to content

Buidl UI for search feature #5835

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
Closed
wants to merge 3 commits into from
Closed

Buidl UI for search feature #5835

wants to merge 3 commits into from

Conversation

@iamonuwa
Copy link
Contributor

@iamonuwa iamonuwa commented Jan 18, 2020

Description

Build UI according to figma design

Refers/Fixes

Fixes #5796

Testing

Click the search on navbar.

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Jan 18, 2020

screen-capture-6

@gitcoinbot gitcoinbot mentioned this pull request Jan 18, 2020
Closed
thelostone-mc
thelostone-mc reviewed Jan 20, 2020
View reviewed changes
thelostone-mc
thelostone-mc reviewed Jan 20, 2020
View reviewed changes
@codecov
Copy link

codecov bot commented Jan 20, 2020 •
edited
Loading

Codecov Report

Merging #5835 into kevin/search will increase coverage by 8.95%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           kevin/search    #5835      +/-   ##
================================================
+ Coverage         17.06%   26.02%   +8.95%     
================================================
  Files               254      253       -1     
  Lines             23725    22082    -1643     
  Branches           3696     3209     -487     
================================================
+ Hits               4049     5746    +1697     
+ Misses            19617    16191    -3426     
- Partials             59      145      +86
Impacted Files Coverage Δ
app/grants/admin.py 45% <0%> (-5.46%) ⬇️
app/chat/tasks.py 25% <0%> (-3.58%) ⬇️
app/dashboard/views.py 9.31% <0%> (-1.77%) ⬇️
app/dashboard/tasks.py 41.17% <0%> (-0.5%) ⬇️
app/marketing/stats.py 0% <0%> (-0.25%) ⬇️
...eting/management/commands/assemble_leaderboards.py 21.08% <0%> (ø) ⬆️
...rketing/management/commands/no_applicants_email.py 0% <0%> (ø) ⬆️
app/app/context.py 0% <0%> (ø) ⬆️
...management/commands/sync_event_bounties_to_chat.py
app/chat/utils.py
... and 69 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 842af8b...7bbd4b0. Read the comment docs.

@owocki
Copy link
Contributor

owocki commented Jan 21, 2020

i've run create_search_results on my local.. and i still cant' get this functionality to work. it never makes any HTTP request to make search queries or shows any results..

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Jan 21, 2020

What error do you get?

@owocki
Copy link
Contributor

owocki commented Jan 21, 2020

no error, just nothing shows up or changes in the search bar

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Jan 21, 2020

no error, just nothing shows up or changes in the search bar

i'll take a look now

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Jan 21, 2020

cc @owocki
I was unable to reproduce the error. It works fine for me.

screen-capture-11

@owocki
Copy link
Contributor

owocki commented Jan 22, 2020

@iamonuwa is that a Search Result from create_search_results? r u able to generate lots of search results?

@owocki
Copy link
Contributor

owocki commented Jan 22, 2020

oh wait i got the profile to work !!! by searching my handle name :)

does it work for all tabs for u?

@owocki
Copy link
Contributor

owocki commented Jan 29, 2020

@iamonuwa any idea when the API returns 100s of results.. why does it not show in the UI https://bits.owocki.com/4guxpA94

@owocki
Copy link
Contributor

owocki commented Jan 29, 2020

two other things that are messed up on this PR (beyond just not being able to load results)

  • search bar isnt mobile responsive
  • this breaks the bounty explorer at /explorer

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Jan 29, 2020

@owocki, I'll make this responsive more. But I'm only using your backend update. I did not make changes to it.

@owocki
Copy link
Contributor

owocki commented Jan 29, 2020

ok well my PR doesnt break the /explorer page, and the results load in all scenarios on my PR... those issues will need to be fixed

@thelostone-mc
Copy link
Contributor

thelostone-mc commented Feb 4, 2020

@iamonuwa let us know when these are fixed and we can get it reviewed + merged

octavioamu
octavioamu suggested changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@octavioamu octavioamu left a comment

Choose a reason for hiding this comment

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

I left some comments, as gitcoin is a big site and "search" word is very common let's use a prefix for this classes naming the parent gc-search and then write css in a component way .gc-search .child-class to avoid collisions

@thelostone-mc
Copy link
Contributor

thelostone-mc commented Feb 11, 2020

@iamonuwa any update on this?

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Feb 11, 2020

I'm on it

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Feb 13, 2020

ok well my PR doesnt break the /explorer page, and the results load in all scenarios on my PR... those issues will need to be fixed

what do you mean by break explorer page?

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Feb 13, 2020 •
edited
Loading

Made changes, sending PR this night

@danlipert
Copy link
Contributor

danlipert commented Feb 18, 2020

@iamonuwa any updates?

@thelostone-mc
Copy link
Contributor

thelostone-mc commented Feb 20, 2020 •
edited
Loading

@iamonuwa ^ could you give us an update ? Else this PR will end up getting stale
Also what is the migration for and lastly how sure are you on the test status of this PR ?

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Feb 20, 2020

I've resolved all the issues raised. See my last commit

@iamonuwa
Copy link
Contributor Author

iamonuwa commented Apr 7, 2020

@thelostone-mc @danlipert @octavioamu @owocki fixed all changes.

danlipert
danlipert suggested changes Apr 8, 2020
View reviewed changes
app/search/migrations/0001_initial.py
@@ -0,0 +1,62 @@
# Generated by Django 2.2.4 on 2020-01-18 15:31
Copy link
Contributor

@danlipert danlipert Apr 8, 2020

Choose a reason for hiding this comment

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

Where did this file come from?

Copy link
Contributor Author

@iamonuwa iamonuwa Apr 8, 2020

Choose a reason for hiding this comment

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

Weird, let me take a look at that

@thelostone-mc thelostone-mc mentioned this pull request Apr 11, 2020
Merged
@thelostone-mc
Copy link
Contributor

thelostone-mc commented Apr 11, 2020

Closing this out and redoing it over at #5835!
Thanks @iamonuwa for getting the base ready for this

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

Reviewers

3 more reviewers

@thelostone-mc thelostone-mc thelostone-mc left review comments

@danlipert danlipert danlipert requested changes

@octavioamu octavioamu octavioamu requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@iamonuwa @owocki @thelostone-mc @danlipert @octavioamu