codeforboston / pantry_pickup

Combining city data with a list of specific needs from food pantries will allow citizens to most effectively make useful and needed donations assisted by the Pantry Pick-Up App.
http://www.pantrypickup.org
21 stars 35 forks source link

mobile, responsive design #26

Closed rlaskey closed 11 years ago

WheresHJ commented 11 years ago

Also, re: "searchIndicatorBar is gone: messages not super helpful + space is important for mobile", that bar was intended to show you your search area as well, so if you searched for "Natick, MA" or "02139" by location, it would display that. Though maybe it's self-evident.

rlaskey commented 11 years ago

Hi, Harlan, that's good to know, re: searchIndicatorBar. The input field, #serachInput, does not get reset, so if you search for any terms like that, it'll stay there, which also functions for display.

Do you think that search form should be persistent in mobile, up top, to make it that a primary function, or conditionally hidden via a toggle?

rlaskey commented 11 years ago

Hi, All! I'm a bit unclear as to what would make this pull request accepted.

The last commit, https://github.com/rlaskey/pantry_pickup/commit/a063aab2cb602d19a6d32467e422df3b96d4beb3, gets us a responsive design, which built on what I had initially sent over.

The outstanding issue, #27, was not from any of this work but from the previous pull request. I think we developed some good ideas of where to go with that issue last night, and when I have some time I'd be happy to hack away at some solutions.

As far as I can tell, I'm on the same page with style and Backbone and direction, so if there's something missing that you'd like to see in order for this to be merged do please holler out.

carpeliam commented 11 years ago

I think the issue here is that there's just a lot going on in this pull request. Can you break it down into multiple issues, with separate pull requests? The way things are at the moment, it's an all-or-nothing thing, it's too much for me to review. (Also, the geocoding issue is still throwing a 400, even though it's an issue that our server is experiencing when talking to the geocoding server- this is not a Bad Request).

This guide might help, with the addendum that Steve Klabnik makes in the first comment about rebasing commits from upstream instead of merging, which creates a ton of nasty merge commits.

rlaskey commented 11 years ago

Cool. I cleaned up a load, and re-worked the hell out my branch. Theres' clearly still a lot here, and it's one request, but I was not able to separate things beyond what's here as everything builds on what happened before. At least, I couldn't get the merges to work any better than this.

I also rebased from the latest changes in the CfB repo, so we should be all set with that.

Honestly, there's still a lot of work here, no matter how you cut it. I understand that it's a lot to go through. I spent a while creating it, too.

If there's more that you think I can separate, or there's anything else I can do, just let me know.

carpeliam commented 11 years ago

Thanks for reworking it as much as you've done so far. One thing that I mentioned earlier is that I'd like the PantryCollection.search method back. And PantryCollection.parse should not be calling functions that do things to views... Backbone events are built around the Observer pattern. Maybe we can do some pair programming on a Tuesday? That might be the easiest way to go.

rlaskey commented 11 years ago

I refactored most of what I did, to create smaller diffs with more atomic commits, and to stick to the patterns you mentioned re: Collection methods. If there's anything else you need just holler out. I have the desired git workflow under control now, which was took me a minute but makes sense to me now.

Here's the part I was missing, which might help others, too: once I had something committed in my fork, I did not know how to successfully break one commit into more than one. My answer was to use a rebase -i, then choosing to edit a commit rather than squash, etc.

Once in that rebase edit state, it worked best to git reset HEAD^. I then had a live diff to work with, at which point I could eventually git rebase --continue. If I wanted to add more commits at that point, though, I'd just do regular git commits before continuing the rebase.

In one case, the merge failed. I then had to check out the conflicted file from a previous commit, and then applied the diff which I extracted from my existing GitHub fork.

carpeliam commented 11 years ago

@rlaskey I've merged this in. I did have to make a few changes based on issues I couldn't find by just looking at the code (e.g. searching through the form was broken as there was no handler that was listening for it- I had to add back in the PantriesView). I also took some of the jQuery click handlers and moved them into their respective view, and cleaned up some of the ID reference code (Backbone collections have a get() method that can search by ID, no need to iterate through them). There's still some additional cleanup that I'd like to make, but let's get together on Tuesday or some day soon and go through this together, I think that'd be more helpful for the both of us.

WheresHJ commented 11 years ago

A few styles seem to have fallen out, possibly as a result of this commit, probably earlier if we were changing classes around and stuff. I'm going to take a peek.

rlaskey commented 11 years ago

thank you, Liam, for merging this in. I appreciate it. I'm following what you're doing so far, and it looks good to me. If you have any questions with what I did, do not hesitate to reach out.

Harlan, similarly, if there's a thing you're looking for, I probably can help find where it went. I did consolidate a lot of the CSS, and it's possible I left something out by accident in the process.