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

Display nodes that have tags #57

Merged
merged 12 commits into from
Feb 27, 2018
Merged

Conversation

mstock
Copy link
Contributor

@mstock mstock commented Feb 22, 2018

Issue #56 requested that nodes should also be displayed. The suggested changes in this pull request add this functionality by placing a marker for nodes that have been created, modified or deleted if the node has any tags.
Since this requires that tags are also part of the data structure that is provided by osm-stream, it currently has no visible effect though, since tag parsing for nodes only becomes available if/when osmlab/osm-stream#13 (or something similar) gets merged, which currently has a syntax error that could be fixed by applying a trivial patch like the one in mstock/osm-stream@56bf6bd. Once that pull request is merged, somebody would have to cut a release of osm-stream and then the corresponding dependency in show-me-the-way would need updating.
The first five commits in this pull request are mainly smaller code cleanups and fixes - I can extract them to a different branch and create a separate pull request or even drop them if you prefer.
For demonstration purposes, I've deployed a build with the node feature that uses a patched version of osm-stream on my Github page.

Should make it easier to start development.
And as a consequence, declare some variables that were not declared, yet.
Since OSM data, changeset comments and potentially even OSM usernames may
contain HTML code, it's better to use the textContent property to insert
such values, since innerHTML would result in interpretation of this HTML
code which might even result in the execution of JavaScript code [1].
The Underscore.js template is also affected by such issues, which can be
mitigated by using <%- instead of <%= for interpolations [2].

[1] https://developer.mozilla.org/en-US/docs/Web/API/Element/innerHTML#Security_considerations
[2] http://underscorejs.org/#template
The 'or' operation is redundant in these cases since both operands are
identical, and can thus be removed.
is_a_way is not modified in the true-branch of the condition, so it's still
true when returning from it and can thus be removed from the expression.
This is more generic and the wiki [1] also talks about elements when
describing ways, nodes and relations. Prefixing 'map' prevents confusion
with DOM elements which are also common in such an application.

[1] https://wiki.openstreetmap.org/wiki/Elements
… generic

As neither change.type, change.old.tags nor change.neu.tags is modified in
the loop, the value of tags will always be the same, so this can be moved
outside the loop.
Instead of using the hard-coded 'a way' text for the fallback tag text,
take the actual element type from the change, which is more generic in case
the element is something which is not a way.
…ield

The type of an element does not change, so this seems a little simpler and
should make it easier to support other element types.
Makes the functionality more reusable for other element types.
Done just like for ways, but with a L.CircleMarker instead of a L.PolyLine.
Also added a few more tags which should be shown that are often used on
nodes, but at least some of them may also be used on ways.
@iandees
Copy link
Member

iandees commented Feb 23, 2018

@mstock thanks for working on this! I tagged a version of osm-stream with the fix for more than ways as 0.0.14. Can you update the package.json here so it uses it, then we can try it out?

This version actually provides tag information for nodes, so with this they
get displayed.
@mstock
Copy link
Contributor Author

mstock commented Feb 23, 2018

You're welcome :). I just pushed another commit which updates osm-stream to osmlab/osm-stream#v0.0.14. Since there does not seem to be a release on NPM, yet, I assume using the Github reference in package.json is the right way to do this.

@iandees
Copy link
Member

iandees commented Feb 23, 2018

Yea, that probably will be ok for now. I'm not sure I have access to the npm project 🤔

@iandees
Copy link
Member

iandees commented Feb 27, 2018

This looks great, thanks!

@iandees iandees merged commit 950f337 into osmlab:gh-pages Feb 27, 2018
@sfkeller
Copy link

Thats very cool!
Thanks Manfred.
Ian: Is it already deployed on osmlab.github.io/show-me-the-way/#bounds=34.67,-11.78,70.55,31.73 ?

@iandees
Copy link
Member

iandees commented Feb 28, 2018

Yep, it should be.

@sfkeller
Copy link

Thanks. It's hard to validate from outside; I'm still waiting to see a node popping up :-).

@mstock
Copy link
Contributor Author

mstock commented Feb 28, 2018

Actually, I don't think it is - the GitHub page uses the 'compiled' bundle.js, and that has not been touched for a few months. I did not include a 'compiled' bundle.js in my pull request, so somebody still has to do that and commit/push the 'compiled' file.
I'm not sure what the right or best workflow is, but I tend to think that it is better to not include a 'compiled' version in pull requests, since this will cause conflicts if multiple pull requests get merged at the same time, as they would all contain a different variant of the 'compiled' file (and to get all the new things, a rebuild after the merges would be required anyway). It could also be abused to smuggle in unwanted stuff (even though the 'compiled' file is not unreadable, there still can be a massive diff that had to be reviewed to be somewhat safe) or it could be out of sync with the rest of the pull request, and it's also not clear if it still can be built on another machine if it is only part of the pull request and not (re)built by a maintainer.
Btw.: I think #56 can be closed once bundle.js got built and published.

@iandees
Copy link
Member

iandees commented Feb 28, 2018

Ah, you're right! Thanks for looking into it. It's been a while since I've made a change on this repo 😄.

I'll re-build bundle.js and push it up for now. In the longer term we could switch over to a replacement for GitHub Pages that will build the bundle.js for us.

@iandees
Copy link
Member

iandees commented Mar 1, 2018

I pushed up the updated bundle, but I haven't seen a node show up yet. Let me know if you find one.

@mstock
Copy link
Contributor Author

mstock commented Mar 1, 2018

I took a look at the 'compiled' bundle.js, and it seems like the build did not include osm-stream 0.0.14 as specified in package.json (but an older version instead), so I guess that your node_modules directory still contained an older version. This was likely either caused by npm install somehow failing to download this version (the syntax I used to specify the version seems to require at least npm 1.1.65), or not running npm install at all before building the bundle.

In order to automate the build with GitHub pages, one could probably use TravisCI - I've never tried this though, and since it requires a personal token, TravisCI will likely get write access to all repositories the creator of the token has access to (so I'd try to create a second GitHub user for this, and only grant this user access to those repositories where this is required - I don't know if this is actually supported though), and it might also be a good idea to start doing development on a different (like master) and protected branch and 'reserve' the gh-pages branch for the build results from TravisCI.

@iandees
Copy link
Member

iandees commented Mar 1, 2018

I pushed the updated bundle.js and am seeing nodes on the deployed site. Looks good!

This highlights how much is happening in OSM, and that this tool might need to group these changes together so that we spend less time jumping around the world.

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.

None yet

3 participants