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

Search ui improvement #3304

Merged
merged 6 commits into from Sep 11, 2018

Conversation

Projects
4 participants
@stefannibrasil
Collaborator

stefannibrasil commented Sep 7, 2018

Moved from #3295

@plotsbot

This comment has been minimized.

Collaborator

plotsbot commented Sep 7, 2018

1 Message
📖 @stefannibrasil Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Sep 7, 2018

@jywarren I got how to add a button the input. I was looking for Bootstrap 4 docs before, that's why it was breaking on mobile xD

so I added the button to the search, as you requested:

desktop

^desktop

mobile

^mobile

So thanks for the feedback!
I addressed the 3 things you suggested there. I believe now it's good to go :)

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 7, 2018

stefannibrasil added some commits Sep 8, 2018

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 10, 2018

This is looking good -- can you upload a final screenshot and we'll try to merge? Thanks!!!

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Sep 10, 2018

sure!

pl1

pl2

pl3

pl4

:) thank you!

@stefannibrasil stefannibrasil moved this from In progress to Being reviewed in RGSoC 2018 Sep 11, 2018

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 11, 2018

Ok, merging now! Thanks and great work!

@jywarren jywarren merged commit 8252c21 into publiclab:master Sep 11, 2018

3 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
danger/danger All green. Good on 'ya.
Details

RGSoC 2018 automation moved this from Being reviewed to Done Sep 11, 2018

@wafflebot wafflebot bot removed the review-me label Sep 11, 2018

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 11, 2018

Hi, is there a "tag" type here? I'm testing at https://stable.publiclab.org and not seeing those in the results for things like "Balloon mapping"...

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 11, 2018

Ah, yes - here

image

Odd that it doesn't show for everything... just some tags!

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Sep 11, 2018

@jywarren Ok, so I have an idea about what may be happening here:

  1. this line returns 5 items in general not for the category as I expected :( . I'll move back to 15. It may be messing with the tags because it is returning only 5 items.

  1. I will change the order of the items added to the SearchService.textSearch_all method so the categories don't get messed like this.

I'll push them in a minute, thanks for the help!!!

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 11, 2018

thank you too!

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Sep 11, 2018

ok, so I opened a new one.

I put some limits on the methods, the results were almost 200! Maybe we can create an issue to standardize those limits, but I guess should make it better for now.

One thing that I don't know is why it isn't grouping the categories. If you search by 'oil' and see the results on Console > Networks, it is grouped by category:

{
  "items": [{
    "docId": 16591,
    "docType": "file",
    "docUrl": "/notes/pfhs/06-29-2018/video-describing-basics-of-launching-regular-mini-balloon-kits",
    "docTitle": "Video describing basics of launching regular/mini balloon kits?",
    "category": "NOTES",
    "docScore": 0
  }, {
    "docId": 16588,
    "docType": "file",
    "docUrl": "/notes/pfhs/06-28-2018/sample-aerial-balloon-or-kite-images-from-mobius-action-camera",
    "docTitle": "Sample aerial (balloon or kite) images from Mobius Action Camera?",
    "category": "NOTES",
    "docScore": 0
  }, {
    "docId": 16426,
    "docType": "file",
    "docUrl": "/notes/pfhs/06-03-2018/how-long-does-an-inflated-neoprene-balloon-last-mylar-balloons-better-for-summer-camp",
    "docTitle": "How long does an inflated neoprene balloon last? Mylar balloons better for summer camp?",
    "category": "NOTES",
    "docScore": 0
  }, {
    "docId": 16357,
    "docType": "file",
    "docUrl": "/notes/tmclean1/05-17-2018/mapknitter-map-of-fontainebleau-state-park-balloon-nir",
    "docTitle": "MapKnitter map of Fontainebleau State Park Balloon NIR",
    "category": "NOTES",
    "docScore": 0
  }
}

Let's see what happens now with those changes and then I can investigate later the groupying problem, @jywarren

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Sep 11, 2018

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 11, 2018

@stefannibrasil

This comment has been minimized.

Collaborator

stefannibrasil commented Sep 11, 2018

yes... they both use the same method from the API to get the results. We can pass the .limitas a param to all the methods, then we can use as we need. I'll do it tonight, or tomorrow.

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