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

add event listener for all_features and add it to the featurelist #98

Closed
wants to merge 2 commits into from

Conversation

Gutschlhofer
Copy link
Contributor

There is currently no way to get all polygons of the group used for editing (incl. the ones not touched by the user). My use-case was a shiny app that displayed the user a set of geometries that could be edited but didn't have to be.

Say I give the user 5 geometries, she/he edits 2, deletes 1, and adds 1. So far I could only get those that he touched (so the 2 edited, 1 deleted and 1 added) but not the others (remaining 2 geometries).

After the user is happy with his edit, I wanted to get all geometries currently displayed to the user in the edit-group.
With the new addition, this is now possible with crud()$all.

@timelyportfolio
Copy link
Contributor

@Gutschlhofer, I certainly see some sense in the pull request and I really appreciate the effort.

I would like to think all the way through this to make sure we don't cause any confusion. One small problem is that leafpm does not provide (see lines) an all event, since leafpm multiple layer support makes this much more difficult. I guess for now we could just leave blank for leafpm.

For the downstream mapedit functions, I don't think this addition causes any problems

@Gutschlhofer
Copy link
Contributor Author

@timelyportfolio, after your reply I played around with if-clauses to exclude it for leafpm, but I think it makes more sense to just keep it as is and return NULL to leafpm users, that way leaflet.extras users will have the nice benefit of this and others won't be disturbed in any way.

@timelyportfolio
Copy link
Contributor

great @Gutschlhofer I plan to review this and other pull requests next week. I really appreciate your efforts.

@Gutschlhofer
Copy link
Contributor Author

That's great to hear! I just stumbled across editor == "leafpm" and changed it to editor[1] == "leafpm" to avoid warnings.

@tim-salabim
Copy link
Member

Shouldn't this usually be matched right at the beginning of the function, as in editor = match.arg(editor) ?

@Gutschlhofer
Copy link
Contributor Author

@tim-salabim thanks for pointing that out! I only now saw that #100 reworks the editor-logic and also introduces the match.arg(editor) so I reverted back to my original commit.

@timelyportfolio
Copy link
Contributor

So sorry this has taken so long. Going to merge into a new develop branch with #97 and #100 and after a short review period merge to master and CRAN over the next couple of days. Thanks so much for contributing. If ok, I will add you to DESCRIPTION as contributor.

@tim-salabim any other comments?

@timelyportfolio
Copy link
Contributor

@Gutschlhofer merged in #107. I really appreciate your contribution. Thanks. Please let us know if you have any other ideas for improvement.

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