chipzoller / hugo-clarity

A theme for Hugo based on VMware Clarity
Other
582 stars 267 forks source link

Shortcode for openstreetmap #264

Closed imranakram closed 2 years ago

imranakram commented 2 years ago

I wanted to add a map view on the about page and managed to do this via openstreetmap. Thought it would be fun to push a PR so it benefits more people. This is my first PR for a Hugo theme.

Changes / fixes

Screenshots (if applicable)

Checklist

Ensure you have checked off the following before submitting your PR.

onweru commented 2 years ago

Thanks @imranakram for contributing, will review this PR asap.

rootwork commented 2 years ago

Four thoughts:

  1. This doesn't just add support for embedding OpenStreetMaps, which is a little unintuitive, but possible directly from OSM. It's adding support for uMap, which is a tool to create maps using OSM, and embed those creations -- in addition to a regular OSM map -- on a site. That seems fine; it definitely looks easier than OSM and obviously has more functionality, but we might want to be clear about what we're adding, and/or change the shortcode if we think we'd want to add direct OSM embedding at some point.
  2. The maps uMap creates seem to (sometimes?) pull in French-language OSM data even when the uMap path includes en. e.g. just clicking to create a sample map took me here which at least for me is showing a map of Europe with French names (Londres, Bruxelles, etc.). That might just be a bug with uMap.
  3. Embedded iframes don't scale great at smaller screens i.e. smartphones. This would be a problem with any embedded map but I don't know if we'd want to add some CSS to try to adjust things. Right now the map just gets cut off; it could also create horizontal scrolling in some environments.
  4. There's one spelling correction that's out of scope, but I don't know how strongly @onweru @chipzoller feel about that.
chipzoller commented 2 years ago

@imranakram can you address and resolve conflicts?

chipzoller commented 2 years ago

@onweru and @rootwork please review and approve or suggest feedback

rootwork commented 2 years ago

It doesn't look like anything has changed, it looks like the branch was just updated with the latest master. Or am I missing something?

chipzoller commented 2 years ago

@imranakram it doesn't appear maintainer comments have been attended to. Can you address or respond?

imranakram commented 2 years ago

Four thoughts:

  1. This doesn't just add support for embedding OpenStreetMaps, which is a little unintuitive, but possible directly from OSM. It's adding support for uMap, which is a tool to create maps using OSM, and embed those creations -- in addition to a regular OSM map -- on a site. That seems fine; it definitely looks easier than OSM and obviously has more functionality, but we might want to be clear about what we're adding, and/or change the shortcode if we think we'd want to add direct OSM embedding at some point.

Yes that is correct!

  1. The maps uMap creates seem to (sometimes?) pull in French-language OSM data even when the uMap path includes en. e.g. just clicking to create a sample map took me here which at least for me is showing a map of Europe with French names (Londres, Bruxelles, etc.). That might just be a bug with uMap.

This seems like a bug on uMaps part as you mentioned.

  1. Embedded iframes don't scale great at smaller screens i.e. smartphones. This would be a problem with any embedded map but I don't know if we'd want to add some CSS to try to adjust things. Right now the map just gets cut off; it could also create horizontal scrolling in some environments.

I agree, I'm not a huge fan of iFrames either, would be nice to have some implementation that avoids it but that's not my forte` 😊

  1. There's one spelling correction that's out of scope, but I don't know how strongly @onweru @chipzoller feel about that.

I basically resolved conflicts in the latest commit

rootwork commented 2 years ago

OK, so then I think it's just a question of how @chipzoller @onweru feel about the first item above; if we should be clear about or change the shortcode name for what's being embedded.

onweru commented 2 years ago

I think this PR looks good. 1) Documentation is good 2) It's a shortcode, anyone who wants to use it will decide if or not they mind the iframe. Merge 👍🏼. Thanks again @imranakram for contributing.

chipzoller commented 2 years ago

Conflicts in README.md to be resolved.

chipzoller commented 2 years ago

@imranakram there are changes requested here

chipzoller commented 2 years ago

Still looking for some updates here.

imranakram commented 2 years ago

hi @chipzoller @rootwork I'm back to this, sorry I had to be away for sometime.

imranakram commented 2 years ago

@chipzoller I've merged with the latest, resolved conflicts and pushed fixes to address the comments. Please have a look :)

chipzoller commented 2 years ago

Ivan has requested changes still and there are comments yet unresolved.