codeforboston / mbta-ninja

MBTA Ninja!
http://mbta.ninja
MIT License
82 stars 46 forks source link

Alerts Map #152

Open aholachek opened 8 years ago

aholachek commented 8 years ago

Hi, here's a quick summary of the major changes:

Please let me know what needs to be changed or improved. I'm happy to work more on it to get it ready for release, this was a fun project that has already taught me a lot about meteor and maps!

radhikamalik commented 8 years ago

Hey @aholachek thanks for submitting the PR! To start, could you please rebase and squash all 3 commits into one and also attach a screenshot of the app as it looks now? Thanks!

aholachek commented 8 years ago

Here's a sample screenshot:

screen shot 2015-11-05 at 7 02 28 pm

I rebased the commits into just the one.

JacobEvelyn commented 8 years ago

This is really cool!

radhikamalik commented 8 years ago

This looks really neat!

I added a few stylistic comments on the code so we ensure it's consistent with the existing code.

I wonder if it makes sense to add a line somewhere (maybe on the map view page?) that this view only supports the main lines (no silver line or commuter rail lines).

Also looking at it on mobile, looks like the map starts out with some unnecessary space at the top above "Oak Grove" (and you have scroll down to see all the lines covered completely). It would be good to get rid of this space at the top so you can see all lines together on the map at a glance. screen shot 2015-11-14 at 5 42 24 pm

Thoughts?

radhikamalik commented 8 years ago

One more thing: we might want to make the buttons to add the reports common to both views so someone doesn't have to necessarily go back to the list view to create a report.

aholachek commented 8 years ago

Hi Radhika, I'm sorry that it took so long for me to reply! Thanks for your comments and suggested fixes. Before I get started making the changes, and maybe a few cosmetic changes to the station markers that I've been thinking about, I just want to confirm that this is a feature that should be added to the project. I went to one of the Tuesday meetups and talked to one of the guys who is a leader of the group and map expert (I've unfortunately forgotten his name); he didn't think the map was necessary. Personally I still think it would be useful, for the reasons I wrote above, but I don't want to force through a feature that has limited utility. I'd love to hear your thoughts.

modestmaya commented 8 years ago

@patrickgreenwell do you know if (at this point in time) there would be any plans to move ahead with the addition of the map feature? Seems like it could be a great feature!