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

Overture sidebar #1644

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Overture sidebar #1644

wants to merge 12 commits into from

Conversation

Bonkles
Copy link
Contributor

@Bonkles Bonkles commented Dec 10, 2024

Even though our data picker allowed you to show overture places, they weren't interactive at all.

This PR adds hover/select capability to all overture places.

There's a new inspector for the sidebar, UiOvertureInspector.js, which is in charge of rendering the places data. This same rendering code should be broadly applicable to other overture datasets too, I stayed away from any places-specific formatting.

Some custom rendering code to process the PMTiles gets a little gnarly, given that the types we're processing are sometimes a single string, dict of strings, or string array, or dict of string arrays. :-/

Here's what it looks like:

image

Copy link
Contributor

@bhousel bhousel left a comment

Choose a reason for hiding this comment

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

Looks good! I will leave some comments inline, but go ahead and merge when you are happy with it!


/**
* UiOvertureInspector
* The OvertureInspector is a UI component for viewing Overture Entities in the sidebar.
Copy link
Contributor

Choose a reason for hiding this comment

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

Love seeing all the documentation, and following the new conventions of using classes and $ for selections makes it easy to see what the code is doing! 👍


// localize logo
$inspector.selectAll('.logo-overture > use')
.attr('xlink:href', `#overture-logo-overture-wordmark${rtl}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this code needs to change a little bit for right-to-left.. This is optional and shouldn't hold up the PR.
We could remove the 'src' attribute up on line 87 (in enter selection), and put some code here that sets the 'src' to use either the normal or rtl version of the logo (in update selection).
We'd need to actually make that logo too, but that's easy.


let key = k;

// Some params come to us via pmtiles with a prepended '@' sign.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why they start with @ ? (I'm new to this and don't know.)


const $$tagEntry = $$propBag.append('div').attr('class', 'property-entry');

if (this._hasJsonStructure(v)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nit, but the way it's written we are double-parsing everything - once to determine if _hasJsonStructure is true/false, then again to actually get the json. It would be more efficient if _hasJsonStructure was instead _getJson and it would always return the parsed json or maybe {} if it failed.

@@ -36,6 +36,7 @@
"dist:svg:rapid": "svg-sprite --symbol --symbol-dest . --shape-id-generator \"rapid-%s\" --symbol-sprite dist/img/rapid-sprite.svg \"svg/rapid-sprite/**/*.svg\"",
"dist:svg:roentgen": "svg-sprite --symbol --symbol-dest . --shape-id-generator \"roentgen-%s\" --symbol-sprite dist/img/roentgen-sprite.svg svg/roentgen/*.svg",
"dist:svg:temaki": "svg-sprite --symbol --symbol-dest . --shape-id-generator \"temaki-%s\" --symbol-sprite dist/img/temaki-sprite.svg node_modules/@rapideditor/temaki/icons/*.svg",
"dist:svg:overture": "shx cp svg/overture/* dist/img/",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be ok to just put the svg into dist/img rather than having this command to copy it over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants