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

Group shields in legend by highway system #773

Merged
merged 4 commits into from
Feb 9, 2023
Merged

Conversation

1ec5
Copy link
Member

@1ec5 1ec5 commented Feb 8, 2023

The legend now incorporates Wikidata labels into its “Route markers” section while generating the section, instead of patching in the labels afterwards. If the Wikidata Query Service response hasn’t come back by the time the section is generated, the legend blows away the section and regenerates it once it receives the response.

When generating the section, entries are now combined with other entries that correspond to the same highway system item in Wikidata. This improves the display of highway systems that haven’t been modeled in as much detail in Wikidata. More detailed highway systems continue to get separate rows with separate labels.

Murfreesboro Guayama

Fixes #771.

@1ec5 1ec5 added bug Something isn't working shields labels Feb 8, 2023
@1ec5 1ec5 self-assigned this Feb 8, 2023
1ec5 added 2 commits February 8, 2023 19:29
After fetching labels from Wikidata, delete and regenerate any pending rows in the shield section of the legend.
@1ec5 1ec5 force-pushed the 1ec5-legend-shield-tn-771 branch from 9eedd40 to 2c3d446 Compare February 9, 2023 03:29
@ZeLonewolf
Copy link
Member

Yessss
image

Issue
image

At this location, console errors:
http://localhost:1776/#map=10/40.6983/-73.958

What happened here?
image

Issue
image

@1ec5
Copy link
Member Author

1ec5 commented Feb 9, 2023

This PR is specifically about grouping shields that belong to the same highway system but not the same network – subnetworks, in other words.

Yessss image

This was implemented as part of #632. (Compare with The King’s Highway in Ontario.)

Issue image

This is an unrelated regression in the shield drawing code: #781.

At this location, console errors: http://localhost:1776/#map=10/40.6983/-73.958

Fixed.

What happened here? image

As of #632, whenever the legend encounters instances of a network with and without a ref, it shows both, since the two are likely to differ visually. None of the routes in the Oklahoma Turnpike system are numbered per se, but the Indian Nation Turnpike was incorrectly tagged as State Highway 375, which is concurrent with the turnpike. This tagging error was fixed in changeset 131,759,548; the fix is awaiting a tile refresh.

We could suppress the duplicate shield whenever the shield definition’s notext property is true, but I think the duplication is a good way of spotting this particular class of tagging errors, which was historically quite common before any renderer consumed road route relations.

Issue image

Also #781.

@ZeLonewolf
Copy link
Member

I concur with leaving #781 issues as a separate work item, and this will help highlight the bug when testing a fix for it.

@1ec5 1ec5 requested a review from jleedev February 9, 2023 18:56
@@ -107,6 +115,9 @@ export default class LegendControl {
for (let data of this.sections) {
let section = this.getSection(data);
if (!section) continue;
if (data.id) {
section.id = `legend-section-${data.id}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole flow is a little hard to read: The sections property is assigned by the caller, one of its items has an id of "shields", and then 569 lines later we query the document for this computed value.

Anyway, LGTM. Something to think about for a future refactor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shield section is different from the rest of the legend in numerous ways, to the point that it requires a completely separate code path to generate it in the first place. I suppose it would be worth trying to consolidate the code paths somehow.

One of the differences is that the descriptions get replaced by Wikidata labels asynchronously. Originally, I intended to implement Wikidata labeling functionality for the whole legend, but I reconsidered in favor of eventually maintaining localizations of the static legend entries in this repository: #744. That work will most likely require us to assign an ID to each of the sections and each of their entries anyways.

Previously, all the description rows’ DOM elements, in every section, were obtained from a querySelectorAll() call in open() then plumbed through to prettifyNetworkLabels(). But there’s nothing particularly special about this selector query. Making the element unique in the DOM allows us to query for it at any time. Instead of consolidating the shield section’s code path with the other section generation code, we could split it out into a separate class in a separate file, making it clearer that the shield section is more involved and independent from the rest of the legend.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#782 tracks refactoring LegendControl to be more digestible.

@1ec5 1ec5 mentioned this pull request Feb 9, 2023
@1ec5 1ec5 merged commit 8e9ce30 into main Feb 9, 2023
@1ec5 1ec5 deleted the 1ec5-legend-shield-tn-771 branch February 9, 2023 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working shields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tennessee state route shields have repetitive entries in legend
3 participants