Skip to content

Conversation

@Mr0grog
Copy link
Contributor

@Mr0grog Mr0grog commented Dec 17, 2014

This is just a start right now and only includes changes to the API (no UI yet).

Some open questions:

  • What should the default page size be? Right now it's 5, but that's almost certainly too small. 10? 20? something else?
  • If no_map is False, should we still be paging? we'll need to modify the API to have a place to put the number of pages available in that case.
  • How do we do error handling in the API right now? page and page_size need to be ints, but int() will raise an exception if it can't parse.
  • Should we preserve a way to get back all results instead of paged results? page=0 or page_size=infinity or who knows what?

@brucegarro
Copy link
Contributor

@Mr0grog Looks interesting so far! I didn't know Django had a built-in paginator.

  1. I'm thinking default paging should be 12 or 16 (the rows are 4 wide).
  2. ?no_map=False should not be paging. The map needs the whole query. Additionally, from a UX point of view the map is being demoted to a side page, so performance is a lower priority.
  3. I wouldn't worry about TypeErrors in the api right now. The Django model.objects api seems to do some auto-casting between strings and ints. Additionally, the page parameter isn't going to be a user configured parameter in most cases. I was imagining the pagination would be done by scrolling down.
  4. I would consider this low priority.

cc: @allthesignals

@allthesignals
Copy link
Contributor

Thank you @Mr0grog for diving in and helping with this task. It's a pretty fundamental feature that we've been lacking for a while.

  1. The rows in the results page can change based on viewport width. Eventually we will have masonry-style results with varying image sizes. The original design shows 15 results so 12 or 16 seems like a good start.
  2. After some conversations tonight, I think we should consider revisiting the role the map plays in the design.

@brucegarro
Copy link
Contributor

  1. Agreed Matt. Were you thinking for it to play or more or less prominent role? Talk to Jesse about it perhaps.

@allthesignals
Copy link
Contributor

@brucegarro I was thinking a more prominent role after talking it over with @Mr0grog & @WheresHJ. I'll talk it over with Jessie when she gets time later this week to work on the CSS.

@allthesignals
Copy link
Contributor

I pulled this into it's own branch on my local repo, so I can work more tomorrow. @brucegarro you might do the same if you get a chance later. Thanks everyone!

@brucegarro
Copy link
Contributor

@allthesignals I think I'm in agreement too. Let me know what you guys come up with.

@allthesignals allthesignals mentioned this pull request Dec 17, 2014
@Mr0grog
Copy link
Contributor Author

Mr0grog commented Dec 17, 2014

@brucegarro: I wouldn't worry about TypeErrors in the api right now. The Django model.objects api seems to do some auto-casting between strings and ints.

Yeah, but sadly, the Paginator doesn’t auto-cast :(

Anyway, if you’re not worried about sending back nice errors from the API right now, that’s cool. I’ll just rip out the FIXMEs.

@brucegarro
Copy link
Contributor

Haha, interesting. I'm guessing pagination won't be a user configurable thing, so we should be alright.

Don't worry too much about the comment cleanliness.

@allthesignals
Copy link
Contributor

Quick note: pagination breaks the old map. This might not be relevant for the new release. But it's something to keep in mind. If we get a chance to refactor, we should version our API.

@Mr0grog
Copy link
Contributor Author

Mr0grog commented Dec 18, 2014

Does the old map specify no_map=true? Otherwise this should be OK.

@allthesignals
Copy link
Contributor

Hi @Mr0grog we did a command line merge of this pull request because we started working from your branch.

@Mr0grog
Copy link
Contributor Author

Mr0grog commented Dec 24, 2014

Cool, let me know if there’s anything else I can do/help with on it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants