Add “Maximum Speed” to analysis sidebar #830
Conversation
It shows the distribution of maximum speeds for all ways on the current route (if that data is available, otherwise it’s summed up under “unknown”). `maxspeed:forward` and `maxspeed:backward` is respected in conjunction with `reversedirection`. Hovering/clicking table rows to highlight matching segments on the route work the identical to the other analysis tables. Additionally, all tags in the analysis tab (way type, surface, smoothness) are translateable now. The values were added to `en.json`. Some HTML is rendered with template literals now, instead of concatenating strings. Variable declarations were changed from `var` to `const`/`let`.
mjaschen
force-pushed
the
feature/maxspeed-analysis
branch
from
06f365a to
50e8550
Compare
js/control/TrackAnalysis.js
Outdated
| * for the whole track | ||
| * - renders statistics tables | ||
| * - create event listeners which allow to hover/click a | ||
| * table row for highlighting matching track segments | ||
| * | ||
| * @param {Polyline} polyline | ||
| * @param {Array} segments | ||
| * @param {Array} segments route segments between waypoints |
There was a problem hiding this comment.
nit: segments route segments? Shouldn't it be route segments alone?
There was a problem hiding this comment.
The first segments refers to the variable name, the three following words are the description.
As per JSDoc a hyphen should be inserted after the parameter name. I'll add this in a follow-up pull request.
js/control/TrackAnalysis.js
Outdated
| wayTagParts[1], | ||
| ]); | ||
| if (tagName.indexOf('maxspeed') === 0) { | ||
| formattedName += ' km/h'; |
There was a problem hiding this comment.
nit: not possible to have it within the i18n content directly?
Eg add sidebar.analysis.data.maxspeed.XXX and in English set to %s km/h?
There was a problem hiding this comment.
Yes, good catch. I'll change it in a follow-up PR.
| analysisSortable[type].sort(function (a, b) { | ||
| return b.distance - a.distance; | ||
| }); | ||
| if (type === 'maxspeed') { |
There was a problem hiding this comment.
nit: probably would have used a switch case for this but not a blocker at all :)
add hyphens between parameter name and description
| if (tagName.indexOf('maxspeed') === 0) { | ||
| formattedName += ' km/h'; | ||
| formattedName = i18next.t('sidebar.analysis.data.maxspeed', { | ||
| maxspeed: wayTagParts[1], |
There was a problem hiding this comment.
You could totally remove the if specific to maxSpeed, and instead have a fallback mecanism:
- try to use the full key
sidebar.analysis.data.highway.footway - if not available, try
sidebar.analysis.data.highway - otherwise simply fallback to untranslated highway type eg
footwayhere
It should cover all cases while being agnostic of the kind of analysis (if there are more analysis coming in the future 😏)
const highwayType = wayTagParts[1];
formattedName = i18next.t(
[
'sidebar.analysis.data.' + tagName + '.' + highwayType,
'sidebar.analysis.data.' + tagName,
highwayType
],
{highwayType: highwayType}
);If a translation key needs the highwayType it can use it, otherwise it can just skip it and it should work just fine.
There was a problem hiding this comment.
I spotted some more possibilities to simplify the translations for the analysis sidebar, but I'd rather create a separate PR as there're changes on places unrelated to this PR.
Can you agree with this?
It shows the distribution of maximum speeds for all ways on the current route (if that data is available, otherwise it’s summed up under “unknown”).
maxspeed:forwardandmaxspeed:backwardis respected in conjunction withreversedirection.Hovering/clicking table rows to highlight matching segments on the route work the identical to the other analysis tables.
Additionally, all tags in the analysis tab (way type, surface, smoothness) are translateable now. The values were added to
en.json.Some HTML is rendered with template literals now, instead of concatenating strings.
Variable declarations were changed from
vartoconst/let.