Refactor and cleanup

[Roboe]

Work done

• Clean code
• Improve implementation of #5: overlays are creating dynamically (arr.reduce()) depending on the state
• Improve implementation of #6 too with arr.filter() and getQueries()

  Demos

• Show all layers by default: https://roboe.github.io/fprsmap/
• Show a single layer (Fairphoners Groups): https://roboe.github.io/fprsmap/?show=communities
• Show multiple layers (meetups and shops): https://roboe.github.io/fprsmap/?show=shops,meetups
• Avoid errors and name duplicities (load Fairphoners with URI garbage): https://roboe.github.io/fprsmap/?show=communities,gar,ba,ge,communities

  Dilemma

Should we keep calling Fairphoners Groups communities or groups? I've implemented #6 with communities, but I can easily change it (just replacing it in line 42) for groups or anything before you merge this PR.

Pros an cons

Pros of this implementation (every one needs actual implementation, and a new issue):

• We can further automatize the JSON collection for every layer just specifying an endpoint String property on the state (yay!)
• We can control which layers to show by default just adding a showByDefault boolean property to the state

[StefanBrand]

Should we keep calling Fairphoners Groups communities or groups?

I think this will become obsolete anyway because we have two different layers now, one for angels and one for meetups.

I'd implement it like this: If((meetup.startdate - today) < 2months): show marker; Pro would be that only active communities (aka meetups) will show up in the map. We could remove the layer communities/groups altogether.

Ps.: Thanks a lot for all the work! I'll need some time to understand everything. :)

[Roboe]

only active communities (aka meetups) will show up in the map

I think you haven't considered the case when a community is not active but some people discover it thorugh the map and the thing start to move again... maybe is better a marker color indicator? Green for active, amber for not-so-active... Also, I don't think it'd be very fair from us to drop the effort some people has put on starting a community by removing their marker... :/

[StefanBrand]

Fair enough! This could be a simple boolean hasMeetupInNextTwoMonths provided by the endpoint database. We should draw up some specification for the database with the info we need...

Edit: Here it is: https://github.com/WeAreFairphone/fprsdb

Am 11.09.2016 um 13:25 schrieb Roberto M.F.:

only active communities (aka meetups) will show up in the map

I think you haven't considered the case when a community is not active but some people discover it thorugh the map and the thing start to move again... maybe is better a marker color indicator? Green for active, amber for not-so-active...

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/newluck77/fprsmap/pull/13#issuecomment-246175343, or mute the thread https://github.com/notifications/unsubscribe-auth/AIiMgTwm_pyAcB0a16CkJMZya5tIv2Tvks5qo-UjgaJpZM4J52dl.

E-Mail ist virenfrei. Von AVG überprüft - www.avg.com http://www.avg.com Version: 2016.0.7752 / Virendatenbank: 4656/12993 - Ausgabedatum: 11.09.2016

[Roboe]

I've set up directories as we talked in #15. What do you think?

[StefanBrand]

I took me over an hour to understand all the new functions (especially .reduce()). But when I noticed that different queries can actually create different properties of an object, it was mind-blowing!!!

Remarks

Instead of calling the array ´groups´ in line 37, we should rather call it ´layers´ as it represents one geographic dataset. Generally the naming of some variables and functions is a bit confusing to me (getMapOverlays vs getMapLayers for example).

Bugs

• One thing doesn't work for me: https://roboe.github.io/fprsmap/?show=angel should produce default, but instead nothing is displayed.
• The getMapOverlays(...) function in line 57 expects 2 objects, but you only hand it one in line 92. I'm not sure if this is a mistake or only my lacking knowledge of JS... ;)

[Roboe]

to understand all the new functions (especially .reduce()) [...] it was mind-blowing!!!

Those functions (map(), filter() and reduce()) are idiomatic JavaScript, and are basic for manipulating data. You can have a single source of data (the state) and avoid a lot of fors (business logic).

Instead of calling the array groups in line 37, we should rather call it layers

Ok, I'm fine with that, :+1: Your semantics arguments are sharp: I had a hard time trying to be coherent with the Leaflet docs (a layer and an overlay are not the same) and English semantics. What alternatives do you suggest for getMapOverlays() and getMapLayers()?

?show=not-a-layer should produce default

Oops. That case wasn't taken into account. Will look at it now, :)

getMapOverlays(...) function in line 57 expects 2 objects

JavaScript's functions signatures can accept as many arguments as you want, or even less than declared. In this case, the second argument is optional. If you see the 62nd line, you could observe that, if the defaultOverlays is falsy, then the value {} (empty object) will be used.

Edit: Also note, lines like the 68th implement the Guard Clause Pattern.

[StefanBrand]

What alternatives do you suggest for getMapOverlays()and getMapLayers()?

Hmmm. We should probably differ between Overlaysand BaseLayers. Both of them are layers, so getMapLayers() is correct. Instead layers should be overlays in most of the cases, I guess.

I think you haven't considered the case when a community is not active but some people discover it thorugh the map and the thing start to move again... maybe is better a marker color indicator? Green for active, amber for not-so-active... Also, I don't think it'd be very fair from us to drop the effort some people has put on starting a community by removing their marker... :/

There is a counter argument: What if a community decides to have a meetup in a different city? We would need two different databases then: One for the communities (with fixed location???) and one for the meetups (location is set at creation). I wouldn't go for this option. Also most of the entries in the address book are single persons. They can apply as Fairphone Angels for their city as well (and this way appear on the map).

I'd favour this workflow: If a community is not active, discuss on the forum, agree on a meetup and then enter it into the database.

PS.: A conflict: What if two markers on two different layers are on top of each other?

[Roboe]

Ok, think we can merge this PR because it's gaining complexity and we are already implementing things on top of this work. So I'll open issues for every unimplemented topic we've talked here, take away the [WIP] tag and then you can merge it and transfer the repo to the @WeAreFairphone organization, @StefanBrand, ;)

[Roboe]

Go ahead! Merge this and transfer the repo, @StefanBrand, :+1: