Map: OSM provider — Synchronize viewport and interactions - #34951
Conversation
There was a problem hiding this comment.
Pull request overview
Synchronizes OSM/OpenLayers viewport state, interactions, focus, controls, and disabled behavior with dxMap options.
Changes:
- Adds bidirectional viewport and click-event synchronization.
- Supports bounds fitting, projections, controls, focus, and disabled state.
- Expands OSM tests and Storybook controls.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
osmTests.js |
Expands OSM integration coverage. |
openLayersMock.js |
Models projections, events, controls, and interactions. |
openlayers.register.js |
Registers additional OpenLayers APIs. |
provider.ts |
Adds the provider focus-update hook. |
provider.dynamic.osm.ts |
Synchronizes widget and engine state. |
provider.dynamic.osm.openlayers.ts |
Implements OpenLayers viewport and interaction behavior. |
provider.dynamic.osm.engine.ts |
Extends the map-engine contract. |
map.ts |
Queues focus-option updates. |
OSMMap.stories.tsx |
Adds interactive OSM Storybook controls. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| const toCoordinate = (api: OpenLayersApi, location: MapLocation): Coordinate => ( | ||
| api.proj.fromLonLat([location.lng, location.lat]) | ||
| const getCoordinateProjection = ( |
There was a problem hiding this comment.
There is a large set of util methods. I'd suggest move them into a separate osm.utils.ts file and cover them by Jest tests.
| const values = Array.isArray(subdomains) ? subdomains : [...(subdomains ?? '')]; | ||
|
|
||
| return values.map((value) => url.replace('{s}', value)); | ||
| return values.map((value) => url.split('{s}').join(value)); |
There was a problem hiding this comment.
{s} is magic constant here. How about extract it to the SUBDOMAIN_PLACEHOLDER or something like that?
| return; | ||
| } | ||
|
|
||
| if (this._container.getAttribute('tabindex') !== this._ownedKeyboardTabIndex) { |
There was a problem hiding this comment.
Why we are looking tabindex from DOM and not from business object?
| const target = getOpenLayersKeyboardTarget(); | ||
| assert.strictEqual(target.getAttribute('tabindex'), null, 'focus is disabled on initialization'); | ||
| map.option('focusStateEnabled', true); | ||
| map._lastAsyncAction.then(() => { |
There was a problem hiding this comment.
Why the internal field uses directly in the test (11 entries)?
| this._keyboardEventTarget.setAttribute('tabindex', '0'); | ||
| } | ||
| this._container = container; | ||
| this._ownedKeyboardTabIndex = container.hasAttribute('tabindex') ? undefined : null; |
There was a problem hiding this comment.
Why we related to the DOM attributes and not to the business logic?
| handlers.viewChange(this._getViewState()); | ||
| }; | ||
|
|
||
| this._eventHandlers = { click, moveEnd }; |
There was a problem hiding this comment.
I'd extract initialization to a separate _initHandlers() method.
| } | ||
| } | ||
|
|
||
| private _disableKeyboardAccess(): void { |
There was a problem hiding this comment.
Focus management code looks like not related only to the osm provider. Should it be on the base level and is the behavior the same with all the providers?
No description provided.