Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Combined people/content inline maps #3090

Closed
jywarren opened this issue Jul 18, 2018 · 38 comments
Closed

Combined people/content inline maps #3090

jywarren opened this issue Jul 18, 2018 · 38 comments
Labels
enhancement explains that the issue is to improve upon one of our existing features help wanted requires help by anyone willing to contribute JavaScript Ruby stale
Milestone
Geographic features

Comments

@jywarren
Copy link
Member

In follow-up for the big Geographic Features list (#1416 ), we still need a "combined" content + people map syntax, like:

[map:all:_latitude_:_longitude_]

https://publiclab.org/wiki/inline-maps doesn't list it, so once we implement, we can add that new syntax.

We'd love to show BOTH nodes and profiles at https://publiclab.org/puerto-rico

It'd be a combination of:

  1. People maps: https://github.com/publiclab/plots2/blob/master/app/models/concerns/node_shared.rb#L220
  2. Note maps: https://github.com/publiclab/plots2/blob/master/app/models/concerns/node_shared.rb#L174

I think we could probably make private methods that did these two so there's no code duplication.

@mridulnagpal or @sagarpreet-chadha interested in this one? Mridul knows this stuff pretty well.

I think the templates themselves are at:
@jywarren jywarren added enhancement explains that the issue is to improve upon one of our existing features help wanted requires help by anyone willing to contribute JavaScript Ruby labels Jul 18, 2018
@jywarren jywarren added this to the Geographic features milestone Jul 18, 2018
@jywarren jywarren mentioned this issue Jul 18, 2018
Open
34 tasks
@jywarren
Copy link
Member Author

I think this may now be possible by using a content map, and specify the PL People layer from LEL to be shown... @sagarpreet-chadha is this right?

@sagarpreet-chadha
Copy link
Contributor

sagarpreet-chadha commented Nov 15, 2019 via email

@sagarpreet-chadha
Copy link
Contributor

sagarpreet-chadha commented Nov 15, 2019 via email

@jywarren
Copy link
Member Author

Ah, but if we do LEL + content, but we also specify the PL People layer in LEL, wouldn't we see both? Thanks!!

@sagarpreet-chadha
Copy link
Contributor

Yes that is correct but we do not have people layer in LEL yet.

@jywarren
Copy link
Member Author

jywarren commented Nov 16, 2019 via email

@jywarren jywarren mentioned this issue Dec 17, 2019
Closed
8 tasks
@sagarpreet-chadha sagarpreet-chadha mentioned this issue Jan 1, 2020
Open
14 tasks
@sagarpreet-chadha
Copy link
Contributor

Ah, but if we do LEL + content, but we also specify the PL People layer in LEL, wouldn't we see both? Thanks!!

@jywarren , should we make a new people Layer in LEL to solve this? Thanks!

@sagarpreet-chadha
Copy link
Contributor

And then we can use:

[map:layers:content:23:77:PEOPLE_LAYER]

What do you think @jywarren ?

@jywarren
Copy link
Member Author

jywarren commented Jan 2, 2020

Ah ok - so i think we need to link to this: #5961

@sagarpreet-chadha what if we thought about @nstjean's work on the /map/_slug_ page and getting people appearing there? And think in parallel about the way to do the inline shortcode?

@nstjean what do you think of this?

Also, #5961 does need some additional work on how the list of people is displayed when you click on a grid square, and perhaps some design thinking about how to explain the grid vs. the markers, maybe a message something like:

Learn about location privacy and why some people are displayed in grid format

within the grid square popup?

Sorry to be putting this info just here, i know this has some break-out tasks to get all these parts working smoothly together.

@nstjean
Copy link
Contributor

nstjean commented Jan 9, 2020

I'll be going through all of this next week! It makes sense to be able to add a content layer and a people layer to get everything to show up!

@sagarpreet-chadha
Copy link
Contributor

sagarpreet-chadha commented Jan 12, 2020 •
edited by nstjean
Loading

Hey @jywarren , @nstjean
Breaking this into small issues:

  • Make a People Layer in LEL.

  • Testing inline map including people Layer using syntax [map:layers:content:23:77:PEOPLE_LAYER]

  • Using LEL embed parameter to load people Layer by default in /map/_slug_ page.

  • Modifying PR PLpeopleLayer added to /people page #5961 to include People Layer of LEL instead of calling API directly.

  • In PR PLpeopleLayer added to /people page #5961 adding message to tell about the grid Vs people relation.

Thanks!

@nstjean
Copy link
Contributor

nstjean commented Jan 13, 2020

@sagarpreet-chadha Are you working on a People layer in LEL or do you want me to do it?

@sagarpreet-chadha
Copy link
Contributor

I can not start immediately 😄 . If this is becoming blocker you can work on it else i will do it by Saturday. Thanks!

@nstjean
Copy link
Contributor

nstjean commented Jan 13, 2020

I'll see if I get to it! I have something in LBL to fix first. :)

@nstjean
Copy link
Contributor

nstjean commented Jan 15, 2020

@sagarpreet-chadha It looks like there is already a People layer in LEL: https://github.com/publiclab/leaflet-environmental-layers/blob/master/src/PLpeopleLayer.js
It was disabled on the example page, but I turned it on and it seems to work (though it is very slow). Is this what is needed?

I am not seeing a PL content layer however.

@jywarren
Copy link
Member Author

We do PL content within plots2, actually -- see these resources --

https://publiclab.org/wiki/inline-maps

This template builds the map:

https://github.com/publiclab/plots2/blob/master/app/views/map/_inlineLeaflet.html.erb

And here's the JS to fetch and display!

plots2/app/assets/javascripts/leaflet_helper.js

Line 94 in c5e3275

function setupInlineLEL(map , layers, mainLayer, markers_hash) {

I hope this helps!

I need to write back to Sagarpreet and you on the people map thing. I think there were some remaining todos with the people map layer in LEL!

@jywarren
Copy link
Member Author

Aha - yes, @sagarpreet-chadha had listed them above!

@sagarpreet-chadha
Copy link
Contributor

Hey @nstjean ,
Yes i remember i had made people layer long way back!
You may not be getting content due to CORS error (check browser console).
Although /people layer in not part of allLayer.js file, right? We can easily add thay there now. Thanks!

@nstjean
Copy link
Contributor

nstjean commented Jan 16, 2020

I don't see an error. And the markers do show up eventually! but every time I move the map it takes a long time for the markers to re-appear. Is that normal?

Peek 2020-01-15 11-35

I don't think it's in allLayers.js so I'll do that. :)

@nstjean
Copy link
Contributor

nstjean commented Jan 16, 2020

I'm getting errors when running tests in master in LEL:

natalie@natalie-ubuntu: ~-Dev-Ruby-public-lab-leaflet-environmental-layers_021

This appears to be the same issue from publiclab/leaflet-blurred-location#232 (comment)

Looks like I'll have to figure out how to get the tests running without a warning in LBL.

@sagarpreet-chadha
Copy link
Contributor

Hey as this PR is old, make sure you are using latest version of LBL. Thanks!

@nstjean
Copy link
Contributor

nstjean commented Jan 20, 2020

I'm definitely using the latest version of LBL, the same test errors are showing for PLpeopleLayer.

@nstjean
Copy link
Contributor

nstjean commented Jan 22, 2020

I think I've solved the testing issues in publiclab/leaflet-blurred-location#235

And then once that fix is published I'll post the PR for showing the PLpeopleLayer. It's still really slow, though, which you can see here: https://publiclab.github.io/leaflet-blurred-location-display/examples/ If you scroll up to the UK/Europe it takes a long while for markers to show up. Is this something I should try to fix in LBLD? Or should I continue on with other items on this list ?

@nstjean nstjean mentioned this issue Jan 22, 2020
Merged
@nstjean
Copy link
Contributor

nstjean commented Jan 30, 2020 •
edited
Loading

I'm making good progress here! Right now in the inline maps we have some that specify layers to display on page load, and some that don't.

  • no layers specified -> display no LEL layers but add all LEL layers to the layer menu
  • specify layers -> display those layers on the map and add only those layers to the menu
    Example: https://stable.publiclab.org/wiki/inline-maps scroll down to "Maps with preset layers" - it only has 2 layers listed in the menu.

My question is should all the layers be added to the menu even when we want only a couple of layers to shown on the map on page load?

@jywarren @sagarpreet-chadha @crisner

@crisner
Copy link
Contributor

crisner commented Jan 31, 2020

My question is should all the layers be added to the menu even when we want only a couple of layers to shown on the map on page load?

Do you mean to specify the layers, load only them to the layer menu but not displayed by default on the map? That is you want the same behaviour from the second point mentioned above but without displaying the maps by default on the page. If this is what you meant, there is an option addLayersToMap , that you can pass in LEL that decides if the layers should be displayed by default on the map when you specify them. It is not available in the version of LEL here in plots2 but will be available in the next version.

@sagarpreet-chadha
Copy link
Contributor

Yes awesome

@nstjean
Copy link
Contributor

nstjean commented Jan 31, 2020

@crisner No, what I mean is loading ALL available layers to the menu, but turning on two of the layers to display just like it does when you use addLayersToMap.

@sagarpreet-chadha
Copy link
Contributor

Oh okay, why do you want to do this?

My question is should all the layers be added to the menu even when we want only a couple of layers to shown on the map on page load?

Yes i guess we can do it.

@crisner
Copy link
Contributor

crisner commented Jan 31, 2020

This might not be good for user experience because loading all layers can slow down the page and the users will be left stuck with no choice to remove it from the map.

@crisner
Copy link
Contributor

crisner commented Jan 31, 2020

@nstjean, What did you have in mind? I mean is there a reason you don't want users to have access to turning off those layers?

@nstjean
Copy link
Contributor

nstjean commented Jan 31, 2020

I think my explanation must be getting confused. I don't mean turning on all the layers. I mean having only the two layers being turned on and visible on the map, but having the other layers available in the menu if the user wants to turn it on.

For example, say in the page only the Unearthing layer is specified. And the user wants to share it to their page, but they also want to add the pfasLayer. Right now there is no way for them to do that, because there is only the Unearthing layer in the menu. That's the only layer they have access to on that map.

FireShot Capture 261 - 🎈 Public Lab_ Inline Maps - localhost

@crisner
Copy link
Contributor

crisner commented Jan 31, 2020

Ah! you can have all the layers added to the menu if you don't pass in the include option. In LEL we use the url hash to determine which layer we want to load on the map by default. Maybe something like that can be done here in plots2?
Or how about this we could add another option in LEL and add some logic into it to get this done. It depends on if we want to keep the current style of displaying only the selected layers in the menu.

@crisner
Copy link
Contributor

crisner commented Jan 31, 2020

In LEL we use the url hash to determine which layer we want to load on the map by default. Maybe something like that can be done here in plots2?

This won't help inline maps. So scratch out what I said about using the url hash.

@nstjean
Copy link
Contributor

nstjean commented Jan 31, 2020

I'll think about how best to do this! We basically just have to initialize a regular map and then turn on a few layers. Maybe an option in LEL would be best. I think the include param makes sense for including ONLY a set number of layers. But we could have one called display maybe that is in the same format, listing which layers to turn on?

@jywarren
Copy link
Member Author

I like this include display convention! Sounds awesome, and thanks for thinking through this distinction!

@jywarren
Copy link
Member Author

And yes, to be honest I think across all maps on PL we want all layers to be /available/ but only some to be /shown/. So this distinction can be really helpful for such variations. Thank you!!!

@nstjean
Copy link
Contributor

nstjean commented Jan 31, 2020

Ok great! I'll add this to LEL today.

@nstjean nstjean mentioned this issue Jan 31, 2020
Closed
@stale stale bot added the stale label Oct 7, 2020
@jywarren
Copy link
Member Author

jywarren commented Oct 8, 2020

I believe this is complete! Thank you!!!

@jywarren jywarren closed this as completed Oct 8, 2020
@publiclab publiclab deleted a comment from stale bot Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement explains that the issue is to improve upon one of our existing features help wanted requires help by anyone willing to contribute JavaScript Ruby stale
Projects
None yet
Milestone
Geographic features
Development

No branches or pull requests
4 participants
@jywarren @sagarpreet-chadha @crisner @nstjean