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.

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

Replace layer control with new layers browser menu #320

Conversation

crisner
Copy link
Contributor

@crisner crisner commented Dec 25, 2019

Fixes #318 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@crisner
Copy link
Contributor Author

crisner commented Dec 25, 2019

This is what I have so far:
layerBrowserMenu

What's left to do:

  • Refactor code
  • More styling on the report button
  • Pull data from the layers information JSON file submitted in PR Collect layer information #317
  • Update style changes

@crisner
Copy link
Contributor Author

crisner commented Dec 26, 2019

Work done:

  • Refactored code to avoid repetition
  • Pulled in sample information from JSON file
  • Updated styles

layerBrowserMenu_v2

Things to do:

  • Update style of data info popup or add a different popup
  • Pull in actual data
  • Show only/filter layers available in the current bounds
  • Add media queries

layerBrowserMenu_v2_dataInfoModal

@crisner
Copy link
Contributor Author

crisner commented Dec 26, 2019

@jywarren, @sagarpreet-chadha, please let me know what you think about what I have done so far. Also, should I wait for PR #317 to be merged or shall I copy the JSON file from it to this PR so that I can use it to pull in information?

Copy link
Collaborator

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow!!!

@sagarpreet-chadha
Copy link
Collaborator

I have not reviewed in depth, will do after the PR is done from your side.
This looks fantastic 😄 !!!
I am just wondering how it will look for small maps on plots2, like profile map or side-bar map?
Also how do we close this control bar as the overlay button is not visible in screenshots.
Thanks!

@crisner
Copy link
Contributor Author

crisner commented Dec 26, 2019

Thanks! I shall attach a gif when I am done or apply this control to the demo file and have it live on my forked LEL repo's gh-pages if that's alright. This just replaces the default leaflet control. So, the behaviour is the same for opening and collapsing with some added features to the layers list. 😄

I am just wondering how it will look for small maps on plots2, like profile map or side-bar map?

I am assuming media queries can help with small embeds? We could hide layer information in small screens as well if it seems too crowded. I will have to make some changes to the overlays object in AllLayers.js file for testing in plots2. I'll work that when this is ready.

@crisner
Copy link
Contributor Author

crisner commented Dec 27, 2019

Work done:

  • Pulled in actual data from JSON file
  • Refactored code
  • Added media queries

Screenshots of screens at different sizes

Screenshots 1440px & 1024px:

1440px 1024px

Screenshots 768px & 425px:
768px 425px

Screenshots 375px & 320px:
375px 320px

Things to do:

  • Update style of data info popup or add a different popup
  • Show only/filter layers available in the current bounds

@sagarpreet-chadha
Copy link
Collaborator

sagarpreet-chadha commented Dec 28, 2019 via email

@crisner crisner changed the title [WIP] Replace layer control with new layers browser menu Replace layer control with new layers browser menu Dec 30, 2019
@crisner
Copy link
Contributor Author

crisner commented Dec 30, 2019

Work done:

@sagarpreet-chadha, This PR is ready for review. However, there are a couple of bugs I'm stuck on. I can work on this depending on your feedback.

The feature is live at: https://crisner.github.io/leaflet-environmental-layers/example/index.html#lat=43.00&lon=-4.07&zoom=3&layers=Standard

The following are the bugs I came across:

  1. The layer gets removed from the list only in the next map event when the layer is turned on.
    I came across this issue when I implemented map.removeLayer(layer) if map.hasLayer(layer) returns true. When the map no longer intersects first the layer is removed from the map but still remains visible in the menu. The layer gets removed from the menu only in the next map movement. Do we need to remove the layer from view when it is filtered out or do we let it show in the view even when the layer is not accessible via the layers menu?

  2. The filtering does not work when the map is first loaded on the page but work fine after reloading.
    I can't seem to figure out the issue. I tried the load map event but there was no change in the issue. I can look into this further tomorrow. Currently, the map shows all layers on page load then actively filters out the layers after reload or map moveend events.

@nstjean
Copy link
Contributor

nstjean commented Dec 30, 2019

This looks really good!!

@crisner
Copy link
Contributor Author

crisner commented Dec 31, 2019

Fixed all bugs! 🎉 😄

P.S:- Currently a layer is completely removed from map view when it is filtered out in the layers menu. I've updated the gh-pages on my local repo to reflect the bug fixes.

Copy link
Collaborator

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!!!

@sagarpreet-chadha
Copy link
Collaborator

Hey @jywarren , if this looks good to you i can bump the version and release it to npm 😄 !

@sagarpreet-chadha
Copy link
Collaborator

I think let's merge this, if there will be any changes by @jywarren we can do that in another PR :)

@sagarpreet-chadha sagarpreet-chadha merged commit fdad928 into publiclab:master Jan 1, 2020
@crisner
Copy link
Contributor Author

crisner commented Jan 1, 2020

Awesome! Thanks, @sagarpreet-chadha!

@jywarren
Copy link
Member

jywarren commented Jan 2, 2020

This is INCREDIBLE! So great!

Congrats on a great job!

I'm wondering if we should open a new issue to collect tiny UI refinements and suggestions. It's a huge project and it'll be great to see how people use it. We also have a https://lookback.io account if you'd like to try recording some user testing to get feedback on this!

@crisner
Copy link
Contributor Author

crisner commented Jan 3, 2020

I'm wondering if we should open a new issue to collect tiny UI refinements and suggestions. It's a huge project and it'll be great to see how people use it. We also have a https://lookback.io account if you'd like to try recording some user testing to get feedback on this!

That's a great idea! https://lookback.io looks interesting! Posting the self-test link from lookback.io to PublicLab.org as a note or here as an issue would be a good idea to get users to test I think? or do we have a list of users, who would be ready for testing, to whom we could send the link to?

@jywarren
Copy link
Member

jywarren commented Jan 3, 2020 via email

@crisner
Copy link
Contributor Author

crisner commented Jan 6, 2020

Awesome!

I am not confident about my content-writing abilities but here's some text I came up with for the prompt. The text prompt could be:

"We have made a layers browser feature to browse and select layers with environmental data based on the location on the map. We would like your feedback to help make it better. Help us test the feature by clicking the link given below. The feature can be accessed by hovering or clicking on the button on the top-right."

Below the prompt, we could provide the lookback.io link with a screenshot of the map with an arrow pointing towards the button. Because the new layers menu is implemented only in the demo page we can share the gh-pages link to the demo page for the users to test.

@jywarren
Copy link
Member

jywarren commented Jan 6, 2020 via email

@crisner
Copy link
Contributor Author

crisner commented Jan 6, 2020

Awesome!

@crisner
Copy link
Contributor Author

crisner commented Jan 6, 2020

Sounds good!

@jywarren
Copy link
Member

jywarren commented Jan 8, 2020

Hi, just wondering - the gh-pages demo doesn't show all layers, and I wondered two things:

  1. should we make the one-line version at https://github.com/publiclab/leaflet-environmental-layers/blob/master/example/oneLinerCodeExample.html be the default demo now, so rename it index.html and move index.html to something like specific-layers.html?
  2. is it not showing all layers because we haven't enabled them all in the example? Let's make our default example show all available layers, and this seems to make the above idea even better, because then as we add layers to the .json file we won't have to update the example; they'll be included automatically!

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace leaflet layer control with new layers browser menu
4 participants