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

GoogleMaps support #7604

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

GoogleMaps support #7604

wants to merge 2 commits into from

Conversation

mdastous-bentley
Copy link
Contributor

No description provided.

@@ -19,6 +19,10 @@ export type ImageryMapLayerFormatId = "ArcGIS" | "BingMaps" | "MapboxImagery" |
/** @public */
export type SubLayerId = string | number;

/** @public */
export type PropertyBagArrayProperty = Array<number|string|boolean>;
export interface PropertyBag { [key: string]: number | string | boolean | PropertyBagArrayProperty };
Copy link
Member

Choose a reason for hiding this comment

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

See if you can avoid reinventing JSONSchema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

export type JSONSchemaType = string | number | boolean | object | JSONSchemaType[];

I need a type representing types that can be directly serialized and can be compared easily. This is why I excluded object, and I didn't want to support array of arrays.

Is there a way to compose type from existing types with typescript ?

Copy link
Member

Choose a reason for hiding this comment

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

Do the google maps property bags definitely conform to those constraints? Do we anticipate other map providers that might not meet those constraints?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently for google it only holds this data structure:

export interface CreateGoogleMapsSessionOptions {
  mapType: GoogleMapsMapType;
  language: string;     
  region: string;    
  orientation?: number;
  layerTypes?: string[];
};

My thinking is that MapLayerSettings's properties are meant to be simple list key,value pairs that can be passed down to the provider, it's not meant to pass complex data structures. If nested objects need to be passed, it can always be simplified into a flat list of values and re-construct inside the provider.

@mdastous-bentley
Copy link
Contributor Author

@pmconne Part of this PR, I need to have an async version of TileTreeReference.addLogoCards, mainly because a request need to Google web API is required to retrieve copyright strings associated with selected tiles. Currently it's being invoked through Viewport.forEachTileTreeRef, which is not async.

My only solution right now, would consist in created an async version of the forEach methods, but I feel like it will result in a ugly API.

Any thoughts?

@pmconne
Copy link
Member

pmconne commented Jan 24, 2025

My only solution right now, would consist in created an async version of the forEach methods, but I feel like it will result in a ugly API.

The forEach methods are a relic of the days when we were still learning how to do TypeScript.
I'd suggest supplanting them with synchronous method(s) that return Iterable<TileTreeReference>. Then the caller can worry about calling async methods on the references, halting iteration partway through, doing something with the return value of whatever he's calling on the references, etc etc.

else if (lhs.length === 0 && rhs.length === 0) {
return 0;
} else if (lhs.length !== rhs.length) {
return Math.abs(lhs.length - rhs.length);
Copy link
Member

@pmconne pmconne Jan 24, 2025

Choose a reason for hiding this comment

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

Why absolute value? It breaks the strict weak constraint.

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.

2 participants