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

Using group optional attribute to create an hierarchical layer tree #355

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jgrocha
Copy link

@jgrocha jgrocha commented May 4, 2015

Hi devs,

I recovered these two classes that help create a layer tree, from an group attribute. I also add an example of usage, to geoext2/examples/tree/.

The only change in the existing code proposed in the PR is an additional protection (if clause) in src/GeoExt/tree/Util.js, updateLayerVisibilityByNode function, to make sure that we have a layer associated with the node before checking the layer's visibility. I think this test is necessary, since we might have nodes in the tree (not leafs) without a layer.

The two classes proposed in this PR are:
GeoExt.tree.LayerTreeBuilder that extends GeoExt.tree.Panel
GeoExt.tree.LayerGroupContainer that extends GeoExt.tree.LayerContainer

Comments are welcome.

@marcjansen
Copy link
Member

I currently don't have time to look into this, sorry. I hope to find time for this next week. Others are free to revoew of course.

@chrismayer chrismayer mentioned this pull request May 12, 2015
9 tasks
}

var groupString = layer.options.group || "";
var group = groupString.split('/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this happen in after it has been ensured groupString !== "" in line 175?

@chrismayer
Copy link
Contributor

Hi @jgrocha,
thanks a lot for your contribution! This seems to be a great addition for the tree package of GeoExt.

Besides some minor issues which I addressed directly as line comments (see above) I have some major points which should be done before we can merge this:

  • When I run your example the group node 'Utilities' does not have a checkbox but when I check an underlying layer it appears. Can you have a look at that?
  • Do you see the chance to add some unit tests?
  • Can you please add some more API doc tags on the config properties and functions in your newly created classes, so the API-documentation will be complete?
  • The customized layer icons for base layers (see example tree.html) are gone in your example
  • To list your example in the API-Docs example section you need to do an entry in https://github.com/geoext/geoext2/blob/master/examples.json

@marcjansen
Copy link
Member

Hey @jgrocha do you think you can tackle the comments of @chrismayer?

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.

3 participants