Skip to content

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
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