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

Add a 'selected' indicator to a selected pantry listing #29

Closed ohnorobo closed 11 years ago

ohnorobo commented 11 years ago

This would be informative now that listing autoupdates on map movement/selection.

My idea is a maroon bar to the left of the listing. (matching the maroon of the selected icon)

carpeliam commented 11 years ago

Maroon bar on the left sounds good, I'd say also change the background color of the selected listing?

We talked about implementation last night, Backbone events make it pretty easy-ish to follow the Observer pattern. Give me a holler if you need help bending backbone to your will.

fabacab commented 11 years ago

So I'm taking a stab at Node.js, Backbone.js, and all the rest of the technology y'all are using for the first time in my life and I figured this might be a good place to start picking through your issue queue. So I did some reading and then some banging my head against the wall and came up with this implementation that teaches the Pantry model whether or not it is the selected item in the collection and sets an appropriate class attribute if it is. The CSS then styles that class as you described, above, @carpeliam.

Sadly, I'm still having some trouble with this implementation because while it sets the class correctly, it doesn't unset it when a different Pantry item is clicked. Or rather, it does, but it only sometimes, and for the life of me I can't figure out why. :( Any help appreciated, as I'm afraid I'm nearing concussion-level head-banging frequency with this at the moment.

Also, I'll happily open a pull request for this if you like.

carpeliam commented 11 years ago

I think that selection is a view property, not a model property (you wouldn't persist whether a model is selected or not; selection is really just a pointer that the view has).

Off the top of my head, I'd say that selection occurs within PantryListingView#showDetails (which maybe should be renamed to #selectPantry, I guess?). This handles selection, but not deselection. I'd handle deselection by firing/listening for an event. The only view that should be concerned with deselection is the one that's already selected. As soon as you're selected, you fire the "I just got selected!" event, and then start listening for the "I just got selected" event. If you're listening to the "I just got selected event" (which you'd pick up on if you were the selected view, but aren't anymore), then you'd deselect yourself (remove the CSS class), and stop listening for that event.

This is off the top of my head (I haven't tried it), but something like this might work:

PantryPickup.PantryListingView = Backbone.View.extend({
  className: 'pantryListItem',
  template: _.template( $('#pantryListingTmpl').html() ),
  events: {
    'click': 'selectPantry'
  },
  selectPantry: function() {
    pantryDetails(this.model);
    this.$el.addClass('selected');
    this.trigger('pantry:selected', this);
    this.on('pantry:selected', function(view) {
      if (this !== view) { // not sure if this check is necessary, really?
        this.$el.removeClass('selected');
        this.off('pantry:selected');
      }
    });
  },
  render: function() {
    this.$el.append(this.template({pantry: this.model}));
    return this;
  }
});

Does that make sense?

fabacab commented 11 years ago

In theory that makes perfect sense. In practice, it doesn't. What I mean is, using the code in your comment, above, I can click a pantry item on the listing view to select that pantry (apply the class, and styling), and then click on the same pantry a second time to deselect itself, but clicking on a second pantry item doesn't deselect the first. Moreover, after deselecting a pantry item, clicking on it again doesn't result in re-applying the correct class and styles to the element. It's like that pantry becomes un-highlight-able.

I guess I just don't yet understand Backbone.js enough to why this code behaves that way.

I think the problem I'm having is that I don't know how to define a single kind of view (i.e., PantryPickup.PantryListingView) that listens for events from other instantiations of the same kind of view. That is, given

PantryPickup.PantryListingView = Backbone.View.extend({…});

how do I listen for the pantry:selected event when that event is fired from another view that was also instantiated as new PantryPickup.PantryListingView?

Just doing

this.on('pantry:selected', …);

from inside the definition of PantryPickup.PantryListingView only seems to listen for events from its own object, not a different instance of the same object.

I tried some variations, including

this.listenTo(this, 'pantry:selected', …);

but none of this seemed to change anything.

carpeliam commented 11 years ago

@meitar I checked in some code in a409836 that manages the backbone part of this, sorry for the handwavy code from yesterday. The thing I had forgotten yesterday was that events don't propagate globally. Here's an example:

viewA = new PantryPickup.PantryListingView();
viewB = new PantryPickup.PantryListingView();

viewA.on('pantry:selected', ...); // listens for when event is triggered on viewA
viewB.trigger('pantry:selected'); // alerts anything listening to event on viewB

Nothing ever happens here, because the event doesn't propagate, it just stays on viewB, and viewA never knows about it. The only place where events propagate is, if you fire an event on a model, the event will propagate up to its collection. So, I fired the event on the selected model, and listened to events on the collection.

I renamed the pantryDetails function to selectPantry, because it's doing a lot more than just showing the details. I put the pantry.trigger code in there as that gets called whether the selection happens in the listings panel, or on the map. Eventually, I want to get rid of the selectPantry function and replace the map code with a Backbone.View, where the map is listening to events in the same way that the listings panel is... I think that'll be a lot less confusing than what's there now, which is kind of a hodgepodge.

The difference between on and listenTo is largely just the scope:

collection.on('pantry:selected', function() {
  console.log(this); // collection
});

view.listenTo(collection, 'pantry:selected', function() {
  console.log(this); // view
});
fabacab commented 11 years ago

Okay, I see what you're doing there. Thanks for the explanation, @carpeliam. That's actually closer to my attempt than I was expecting, so I guess that's nice to see. FWIW, here's a pull request with the CSS from my own branch.

On an unrelated note, I also tried just now to make it such that when you click on a pantry in the map view, the correct item in the list view on the left-hand scrolls into view, but I just started banging my head against the wall with Backbone again and got frustrated.

Maybe this just isn't the project for me. I'm really displeased with this Backbone stuff. Good luck!

carpeliam commented 11 years ago

@meitar I think it'll be simpler to understand when it's not a mix of global functions and event-oriented programming. Right now, it's still straddling the line. Thanks for the pull request!