-
Notifications
You must be signed in to change notification settings - Fork 16
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
First pass / rough draft of HTMLMapmlViewerElement.matchMedia API, #1008
base: main
Are you sure you want to change the base?
Conversation
49f37a9
to
a5e3c89
Compare
depends on media-query-parser and media-query-solver.
prefers-color-scheme,map-projection,map-zoom working up to the initial matches. Tbd if this will need modification to support dynamic evaluation of map properties to support attaching event handlers. Not yet determined: how to implement map-extent support, because media features are all either discrete or range types, and while it may be possible to implemnent a bbox using four corner values as the pre-determined corners of the bbox, it is less clear how to represent a bbox as a media feature value, which does not typically contain commas.
(relies on M.options.contentPreference being an array).
allowing dynamic use of the object in event handlers. Add window.matchMedia query + event listener for color-scheme changes, allows map to adapt without having to shake it.
872d89f
to
a6412d0
Compare
…age, not navigator.languages, because other values aren't of use).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, see suggestions/comments.
</script> | ||
|
||
<mapml-viewer projection="OSMTILE" zoom="14" lat="45.406314" lon="-75.6883335" controls="" | ||
controlslist="geolocation"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest to remove controlslist YAGNI
.parseFromString(f, 'text/html') | ||
.querySelector('map-layer'); | ||
map.appendChild(layer); | ||
return { matcher, logResults }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest to rename logResults to hideUnhideLayer or something along those lines. Function doesn't really need to return anything.
</mapml-viewer> | ||
|
||
<div class="description"> | ||
<button class="getBoundingBox">Get Bounding Box</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say this is "the hard way" because it makes you have to click the button in the test. The DOMContentLoaded event handler could do it (although the viewer does take some time to get ready, so maybe the button clicking just works around that?).
<map-input name="z" type="zoom" value="18" min="0" max="18"></map-input> | ||
<map-input name="x" type="location" units="tilematrix" axis="column" min="0" max="262144"></map-input> | ||
<map-input name="y" type="location" units="tilematrix" axis="row" min="0" max="262144"></map-input> | ||
<map-link rel="tile" tref="https://tile.openstreetmap.org/{z}/{x}/{y}.png"></map-link> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been starting to create tests that have no tiles in them at all, so that there's no network waiting going on, or minimal if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By that, does it mean we just won't have a base map for test cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we'll have a map, but the layer will be sort of fake, in that the tref attribute should point to nothing. Let me see if I can find an example...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example: https://github.com/Maps4HTML/MapML.js/blob/main/test/e2e/elements/map-layer/layer-extent.html the tiles just return 404 not found, but it doesn't harm the test afaik
// upon creation, the test query layer should not be hidden | ||
await expect(layer).not.toHaveAttribute('hidden'); | ||
|
||
// the test query layer should be hidden as the bound just overlaps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's hidden because zoom = 14, not because of overlap
await expect(layer).not.toHaveAttribute('hidden'); | ||
|
||
// compute starting and ending points for moving the map | ||
const boundingBox = await map.boundingBox(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are fine as is, but you can also use HTMLMapmlViewerElement.zoomTo(lat,lon,zoom) to move the map and it's just as good, less trouble if you don't need the interactive stuff.
<map-layer id="OSMTILE" label="OpenStreetMap" checked=""> | ||
<map-link rel="license" title="© OpenStreetMap contributors CC BY-SA" | ||
href="https://www.openstreetmap.org/copyright"></map-link> | ||
<map-extent units="OSMTILE" checked="checked"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes I will use actual layers in my html development, then comment those layers out when I commit the test, replacing them with dummy layers that don't fetch anything (for speed, eliminating test dependencies and so on).
</div> | ||
</body> | ||
|
||
</html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<map-link rel="tile" tref="https://tile.openstreetmap.org/{z}/{x}/{y}.png"></map-link> | ||
</map-extent> | ||
</map-layer> | ||
<map-layer media="(map-projection: CBMTILE)" id="CBMTILE" checked> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should take the media attribute off here, it may be confusing to future us
depends on media-query-parser and media-query-solver.