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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show number of active layers on a badge #269

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

crisner
Copy link
Contributor

@crisner crisner commented Oct 13, 2019

Fixes #201 (<=== 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 Oct 13, 2019

The feature is complete. There is a bug where if I move away from the leaflet layers control before the markers load, the badge is hidden once they are loaded. I shall look over my code to correct the bug and update the PR.

Screenshot of the working feature:
127 0 0 1_5500_example_index html (6)



map.on('layeradd', function(ev) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets also add all this code in the /src folder . We want this feature to work when user uses our allLayers feature which automatically adds the menu bar. Makes sense?
This file : https://github.com/publiclab/leaflet-environmental-layers/blob/master/src/AllLayers.js gives the user the ability to add multiple layers. Lets add here also. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's create a function of this and call here in example/layers.js and here https://github.com/publiclab/leaflet-environmental-layers/blob/master/src/AllLayers.js .

color: black;
font-weight: bold;
font-size: 1.4em;
margin-left: 80%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you test if this works on different screen sizes and share screenshot also :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally slipped my mind. Will make sure to test it and share the screenshots.

margin-left: 80%;
position: absolute;
text-align: center;
top: 9px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This hard-coded css also, i am not sure if it will work on all screen sizes .

@@ -0,0 +1,25 @@
L.Control.Badge = L.Control.extend({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add this badge functionality to Readme as well. Telling that this is automatically there if you use
L.LayerGroup.EnvironmentalLayers().addTo(map);

otherwise using the above function (which calls listeners) you can add!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay 馃憤



map.on('layeradd', function(ev) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's create a function of this and call here in example/layers.js and here https://github.com/publiclab/leaflet-environmental-layers/blob/master/src/AllLayers.js .

@crisner
Copy link
Contributor Author

crisner commented Oct 14, 2019

  • Fixed the bug and made requested changes
  • Checked badge in different screenshots and did not find an issue. Screenshots attached below

Screenshot for screensize 320x568:

The overlapping of the leaflet control is not created by the badge.
Screenshot before introducing badge into the code:

Screenshot for screensize 360x640:

Screenshot for screensize 425x725:

Screenshot for screensize 640x360:

Screenshot for screensize 768x1024:

Screenshot for screensize 1024x1366:

@crisner crisner changed the title [WIP] Show number of active layers on a badge Show number of active layers on a badge Oct 14, 2019
@crisner
Copy link
Contributor Author

crisner commented Oct 14, 2019

@sagarpreet-chadha Please review changes and let me know if I have missed anything. Also, please let me know if I have included the information in the Readme file in the right format. Thank you.

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.

Just 1 last change. Thanks :)

@@ -29,4 +29,20 @@ z-index: 10; }
padding-bottom: 25px;
margin-right: 65px;
display: inline;
}

.badge {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, the PR is all set. Only thing is that i think we should move this CSS to /src folder and on grunt build -> we should copy this from /src to /dist folder if it not already there. Makes sense? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Shall work on it. Thanks!

README.md Outdated
@@ -241,3 +241,11 @@ We're going to try spinning this out into its own library; see: https://github.c
hash: true, // by default this is FALSE
}).addTo(map);

## Add badge to display number of active layers

// To add the badge
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we can mention the link to CSS file in /dist folder.

@crisner
Copy link
Contributor Author

crisner commented Nov 4, 2019

@sagarpreet-chadha Made changes requested. Please review.

@jywarren jywarren changed the base branch from master to main June 24, 2020 14:47
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.

Add a badge on layer button to show number of active layers
2 participants