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

Adding feature to make individual groups collapsable #46

Open
wants to merge 7 commits into
base: gh-pages
Choose a base branch
from

Conversation

ForgottenLords
Copy link

@ForgottenLords ForgottenLords commented Mar 16, 2017

I have found that while using this library, I sometimes end up with an extremely large list of layers that can be added to a (sometimes quite small) map. The best solution I have found for dealing with this is to make each group of layers individually collapsible, allowing a user to quickly and easily locate the layers they are interested in without being overwhelmed by the full list of layers.

I have added the following options:
groupsCollapsable: defaults to False. If True, all groups are collapsible and start off collapsed.
groupsCollapsedClass, groupsExpandedClass: The css class(es) used to control the toggle icon for each group. Defaults to showing a simple + or - icon, but works with custom classes such as "glyphicon glyphicon-chevron-right" to use bootstrap glyphicons.

collapsable groups
Example of usage with Bootstrap Glyphicons (Not dependent upon bootstrap)

I discovered the need for this independently from Issue #20 which I discovered after making this pull request.

Added a config option "groupsCollapsable" which if set to true, will
make each group header individually collapsable.  This is useful when
dealing with either small map elements, or a larger number of layers
within the groups.

Also added two supporting config settings: "groupsCollapseClass" and
"groupsExpandClass" which control the css styles used for the toggle
indicators, allowing users to override the default +/- indicators.
Setting one to "glyphicon glyphicon-chevron-right" will use the
glyphicon provided with bootstrap for example.
Prevented event propagation from the label to the checkbox when group
checkboxes are enabled.  Cleaned up some code.
this.layers is an undefined property of the GroupedLayerControl object,
replaced with this._layers.

Modified method of deleting an array item to correctly remove the
deleted index so that when you iterate over the _layers array in the
.update() function, there are no undefined/empty slots in the array that
will break the _addItem function.
@@ -66,7 +69,7 @@ L.Control.GroupedLayers = L.Control.extend({
var id = L.Util.stamp(layer);
var _layer = this._getLayer(id);
if (_layer) {
delete this.layers[this.layers.indexOf(_layer)];
this._layers.splice(this._layers.indexOf(_layer), 1);
Copy link
Author

Choose a reason for hiding this comment

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

This is a bug fix independent from my collapsable groups feature.

Copy link
Owner

Choose a reason for hiding this comment

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

Nice catch! Someone else opened a PR for this which I just merged (#50). Can you take this commit out of your branch?

@nadnerb33
Copy link

Thanks for this, works great and also fixes the error I had where some layers can't be removed from the Layer Control.

Copy link
Owner

@ismyrnow ismyrnow left a comment

Choose a reason for hiding this comment

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

Thanks for this feature. Two main things:

  1. Would this require changes for anyone already using the library who upgrades to this version? If so, I'll want to publish this as a new major version. Not a big deal to do so, just important for me to know about when publishing to npm.

  2. Can you update the docs (readme) and add an example (or update the examples) for how to use this? A new example with the glyphicons class thing you worked out would be really helpful for some people.

Overall, I think a lot of people would benefit from this.

@@ -66,7 +69,7 @@ L.Control.GroupedLayers = L.Control.extend({
var id = L.Util.stamp(layer);
var _layer = this._getLayer(id);
if (_layer) {
delete this.layers[this.layers.indexOf(_layer)];
this._layers.splice(this._layers.indexOf(_layer), 1);
Copy link
Owner

Choose a reason for hiding this comment

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

Nice catch! Someone else opened a PR for this which I just merged (#50). Can you take this commit out of your branch?

.leaflet-control-layers-group.group-collapsable.collapsed .leaflet-control-layers-group-collapse,
.leaflet-control-layers-group.group-collapsable:not(.collapsed) .leaflet-control-layers-group-expand,
.leaflet-control-layers-group.group-collapsable.collapsed label:not(.leaflet-control-layers-group-label){
display: none;
Copy link
Owner

Choose a reason for hiding this comment

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

Can you switch to two-space tabs please? I know the repo doesn't have anything fancy to check for that (and might even have some inconsistencies due to other PRs 🙈 ), so try to match the existing indentation whenever possible.

Copy link
Author

@ForgottenLords ForgottenLords Oct 12, 2017

Choose a reason for hiding this comment

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

  1. I designed the improvements to be disabled by default so that it doesn't cause problems with older configurations, so upgrading should have no problems.

  2. I will try finding time in the next few days to revisit the code and make the requested changes.

Changed from four-space indenting to two-space to conform to upstream
style
Added documentation to the readme, and two examples of collapsability.
@ForgottenLords
Copy link
Author

ForgottenLords commented Oct 30, 2017

I have finished updating the pull request.

  1. The new features added are entirely optional and no existing implementations will be impacted.

  2. I have added a section to the readme explaining how to enable the feature if they desire

  3. I have provided 2 examples, a basic and advanced version to show how to use bootstrap classes to override the default behavior

  4. I have fixed the indenting to conform with your preferred two-space method

  5. I have not removed the fix for removeLayer() as you requested because the current implementation/previous fix is still incorrect as I explain in a separate review.

If you require any further changes please let me know and I should be able to make it happen.

Copy link
Author

@ForgottenLords ForgottenLords left a comment

Choose a reason for hiding this comment

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

The fix from #50 is incorrect, as they use the delete keyword to remove an item from the _layers array, this will result in the array looking like [{object}, {object}, undefined, {object}] and when iterating through it elsewhere within your code (like in _update), it will break when it expects to find an object and instead gets undefined.

The splice method should be used instead.

NHellFire added a commit to NHellFire/leaflet-groupedlayercontrol that referenced this pull request Feb 14, 2018
ismyrnow#46 squashed + lint fixed

    Collapsable Groups.

    Added a config option "groupsCollapsable" which if set to true, will
    make each group header individually collapsable.  This is useful when
    dealing with either small map elements, or a larger number of layers
    within the groups.

    Also added two supporting config settings: "groupsCollapseClass" and
    "groupsExpandClass" which control the css styles used for the toggle
    indicators, allowing users to override the default +/- indicators.
    Setting one to "glyphicon glyphicon-chevron-right" will use the
    glyphicon provided with bootstrap for example.
NHellFire added a commit to NHellFire/leaflet-groupedlayercontrol that referenced this pull request Feb 14, 2018
ismyrnow#46 squashed + lint fixed

    Collapsable Groups.

    Added a config option "groupsCollapsable" which if set to true, will
    make each group header individually collapsable.  This is useful when
    dealing with either small map elements, or a larger number of layers
    within the groups.

    Also added two supporting config settings: "groupsCollapseClass" and
    "groupsExpandClass" which control the css styles used for the toggle
    indicators, allowing users to override the default +/- indicators.
    Setting one to "glyphicon glyphicon-chevron-right" will use the
    glyphicon provided with bootstrap for example.
@JohnnyMoonlight
Copy link

Can we make this pull request a thing? If help is needed, please give me a heads up - i would like to use such a feature!

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.

4 participants