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

link to page descriping tag on OSM wiki if able (currently it leads to page describing key) #2754

Closed
matkoniecz opened this issue Aug 18, 2015 · 4 comments

Comments

@matkoniecz
Copy link
Contributor

see for example https://www.openstreetmap.org/edit?editor=id#map=18/52.61392/24.36614 and select object.

In section "All tags" select info button (with "i" icon).

"View on OpenStreetMap Wiki" links page describing key, even for tags where there is a separate page for tag value.

For example for landuse=forest http://wiki.openstreetmap.org/wiki/Tag:landuse%3Dforest is preferable over current target - http://wiki.openstreetmap.org/wiki/Key:landuse .

@ghost
Copy link

ghost commented Sep 2, 2015

I'm working on this issue, and I have a solution working but I am not sure the code is acceptable. Could @jfirebaugh and @bhousel give some advice?

Highlights of the changes for the devs:
iD.TagReference→load→(anonymous callback function) heading changed to
function(err, docs, softfail) {.
softfail is a flag that signals to this function that if it doesn't find appropriate documentation in the response, it should return false instead of displaying a message to the user.
The return value is picked up by a "callback decorator" in iD.taginfo.docs. If it receives false from the real callback function, it omits the 'value' property from parameters and calls taginfo.docs again.

The callback decorator just wants to inject the softfail argument, but it has to know the arguments that it should pass on to the real callback. It is written as

function(err, data) {
    var docsFound = callback(err, data, true);
    if (!docsFound) {
        taginfo.docs(_.omit(parameters, 'value'), callback);
    }
}

Alternatively one could write

function() {
    var args = _.clone(arguments);
    args[2] = true;
    var docsFound = callback.apply(null, args);

or

function(results) { // results = {err:err, data:data}
    var docsFound = callback(_.extend({
        softfail: true
    }, results));

and make all necessary changes.

Is the general direction all right? Would one of the ways to signal a soft-fail be a good one? If not, what would you recommend?

Thanks.

@bhousel
Copy link
Member

bhousel commented Sep 4, 2015

Looks like a good approach me. I guess you have to change here too to pass the tag value?

The first variation of the decorator looks most like how we do this elsewhere in iD code.

@ghost
Copy link

ghost commented Sep 4, 2015

I guess you have to change here too to pass the tag value?

Yes, and also a true that I called fallbackToKeyIfNoValueDocs in taginfo.docs. I couldn't think of a shorter name that makes it clear what it is supposed to do. Any ideas or is it fine?

@bhousel
Copy link
Member

bhousel commented Sep 4, 2015

I would just change

iD.ui.TagReference = function(tag, context) {

to

iD.ui.TagReference = function(key, value, context) {

and do key-only documentation lookup if value is something that can't be reasonably used.

MarianoTucat added a commit to skedgo/iD that referenced this issue Sep 10, 2015
* upstream/master: (346 commits)
  Add test case for callback function returning false
  Correct coding style
  Make raw tag editor show docs for key=value (closes openstreetmap#2754)
  Improvements to access field (closes openstreetmap#2763)
  Disable fullscreen button, add keyboard shortcuts
  update tests to reflect changes in dd32ec3
  Correct API version in osmChange and changeset XML
  Fix "terms" translations (closes openstreetmap#2756)
  Remove Raven code (closes openstreetmap#2769)
  Less strict polygon intersection test in findOuter (closes openstreetmap#2755)
  Use different leaf_cycle/leaf_type for singular tree (closes openstreetmap#2753) And don't show "mixed" options for singular tree (closes openstreetmap#2752)
  Change caption "Access" -> "Allowed Access" (closes openstreetmap#2761)
  Fix broken link and other help improvements (closes openstreetmap#2760)
  don't try to label if centroid is undefined
  Remove unnecessary PhantomJS install step (as mocha-phantomjs is already included as dev dependency)
  switch jshint to eslint (closes openstreetmap#2733)
  Add make rule to npm install maki
  Replace 'X' with Cancel button on save panel (closes openstreetmap#2378)
  Add recycling:glass_bottles, recycling:plastic (closes openstreetmap#2730)
  Add preset for leisure=bowling_alley (closes openstreetmap#2734)
  ...
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

No branches or pull requests

2 participants