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

Team mapml refactoring #515

Merged
merged 99 commits into from
Sep 23, 2021
Merged

Conversation

prushforth
Copy link
Member

Fixes issue #511

ben-lu-uw and others added 30 commits September 14, 2021 09:07
@prushforth prushforth marked this pull request as ready for review September 23, 2021 14:31
@prushforth
Copy link
Member Author

@Malvoz @erictheise fyi this is a big change, hopefully we've covered it all. Let us know if you have concerns. We'll push the change on geogratis and experiments when this gets merged.

@ahmadayubi
Copy link
Member

Should <layer-> be changed to <map-layer>?

@prushforth
Copy link
Member Author

Should <layer-> be changed to <map-layer>?

I believe it would be quite disruptive, and the gain would be a bit more coherence. I tend towards "no, not enough benefit", but remain open to opinions.

@Malvoz
Copy link
Member

Malvoz commented Sep 23, 2021

If we want to be consistent we'll also have to change <mapml-> to <map-mapml> which reads a bit funny. I personally squinted at <layer-> the first time I saw it, while it is valid HTML it looks a bit awkward before getting accustomed to. We should consider if it may affect how developers, especially beginners, perceive them. And if we're to make the changes, they should probably happen in this PR.

I don't have a strong opinion on this, I'm mostly happy the polyfill is heading towards web platform compatibility. 😊

@ahmadayubi
Copy link
Member

ahmadayubi commented Sep 23, 2021

Line 965 in MapLayer.js, it's creating input elements for an extent, I believe they should be <map-input> rather than <input>. Although the fact that it didn't cause errors may mean it's either not a function being used anymore or it's not tested for.

@prushforth
Copy link
Member Author

prushforth commented Sep 23, 2021

Although the fact that it didn't cause errors may mean it's either not a function being used anymore or it's not tested for.

I think it's this, but I'll remove that code once we've got this issue closed. Should have done it before but tempus fugit :-)

@prushforth prushforth merged commit c67198b into Maps4HTML:master Sep 23, 2021
@prushforth prushforth deleted the team-mapml-refactoring branch September 23, 2021 20:30
<map-title>Canada Base Map - Transportation (CBMT)</map-title>
<map-meta http-equiv="Content-Type" content="text/mapml;projection=OSMTILE"></map-meta>
<map-meta charset="utf-8"></map-meta>
<map-link rel="license" href="https://www.nrcan.gc.ca/earth-sciences/geography/topographic-information/free-data-geogratis/licence/17285" map-title="Canada Base Map © Natural Resources Canada"></map-link>
Copy link
Member

Choose a reason for hiding this comment

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

@prushforth I'm looking to reflect these changes in https://github.com/Maps4HTML/MapServer-documentation/tree/mapml, I'm wondering whether it was intended to change the title attribute on <map-link>(s) to map-title as well.

Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming that was not intended, as it seems no other attributes use the map- prefix.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct. We only changed the elements' prefix, I'll await further information as to whether it's necessary to change attributes. Fwiw I think the link title attribute would be appropriate if there's a conflict there that might be motivation to adopt map-title. We haven't done that yet, so I think it's best to keep with title for now. Btw I can't find the mapserver MapML code, I was looking to update it too. Lete know if you come across it.

Copy link
Member

Choose a reason for hiding this comment

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

MapServer doesn't actually support MapML yet, here's the implementation ticket: MapServer/MapServer#6024.

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.

None yet

5 participants