[openstreetmap/openstreetmap-website] Add Communities page (#3301)

[Tom Hughes]

@tomhughes requested changes on this pull request.

Beyond my specific technical points that I've raised I guess my main question is whether loading about 680 records and potentially 37000 translations (currently) is a reasonable way of displaying a list of 17 local chapters...

> @@ -0,0 +1,26 @@
+module OsmCommunityIndex
+  def self.add_to_i18n
+    communities = Community.all
+    files = Dir.glob(Rails.root.join("node_modules/osm-community-index/i18n/*"))

I would make that match `*.yaml` as an extra protection.

> @@ -0,0 +1,26 @@
+module OsmCommunityIndex
+  def self.add_to_i18n
+    communities = Community.all
+    files = Dir.glob(Rails.root.join("node_modules/osm-community-index/i18n/*"))
+    files.each do |file|
+      locale = File.basename(file, ".yaml")
+      community_locale_yaml = YAML.safe_load(File.read(file))[locale]
+      # rails wants en-GB but osm-community-index has en_GB
+      locale_rails = locale.tr("_", "-")

Presumably this affects more than just `en-GB` so maybe the comment should be more generic, like `language-COUNTRY` or something?

Are we sure this is the only mismatch? Has anybody done a complete cross check between our language list and the OCI one?

> +    communities = Community.all
+    files = Dir.glob(Rails.root.join("node_modules/osm-community-index/i18n/*"))
+    files.each do |file|
+      locale = File.basename(file, ".yaml")
+      community_locale_yaml = YAML.safe_load(File.read(file))[locale]
+      # rails wants en-GB but osm-community-index has en_GB
+      locale_rails = locale.tr("_", "-")
+      data = {}
+
+      communities.each do |community|
+        id = community[:id]
+
+        strings = community_locale_yaml[id] || {}
+        # if the name isn't defined then fall back on community,
+        # as per discussion here: https://github.com/osmlab/osm-community-index/issues/483
+        strings["name"] = strings["name"] || community["strings"]["name"] || community["strings"]["community"]

We should be consistent in whether we index `community` using symbols or strings - we use strings here but a symbol a few lines above.

In fact given community is a model can't we use member syntax? So `community.id` and `community.strings`?

> @@ -0,0 +1,26 @@
+module OsmCommunityIndex
+  def self.add_to_i18n
+    communities = Community.all
+    files = Dir.glob(Rails.root.join("node_modules/osm-community-index/i18n/*"))
+    files.each do |file|
+      locale = File.basename(file, ".yaml")
+      community_locale_yaml = YAML.safe_load(File.read(file))[locale]
+      # rails wants en-GB but osm-community-index has en_GB
+      locale_rails = locale.tr("_", "-")
+      data = {}
+
+      communities.each do |community|

I'd use `each_with_object` here rather than initialising `data` before and then using it in the loop.

> +    files.each do |file|
+      locale = File.basename(file, ".yaml")
+      community_locale_yaml = YAML.safe_load(File.read(file))[locale]
+      # rails wants en-GB but osm-community-index has en_GB
+      locale_rails = locale.tr("_", "-")
+      data = {}
+
+      communities.each do |community|
+        id = community[:id]
+
+        strings = community_locale_yaml[id] || {}
+        # if the name isn't defined then fall back on community,
+        # as per discussion here: https://github.com/osmlab/osm-community-index/issues/483
+        strings["name"] = strings["name"] || community["strings"]["name"] || community["strings"]["community"]
+
+        data.deep_merge!({ "osm_community_index" => { "communities" => { id => strings } } })

Does this actually need the outermost `{}` around the hash?

> @@ -0,0 +1,26 @@
+module OsmCommunityIndex
+  def self.add_to_i18n
+    communities = Community.all

Tricky question, but should we limit this using the same filter we use when choosing what to display? It would vastly limit how many translations we are pulling in...

> @@ -7,6 +7,7 @@
     "js-cookie": "^3.0.0",
     "leaflet": "^1.6.0",
     "leaflet.locatecontrol": "^0.75.0",
+    "osm-community-index": "^5.1.3",

Well yes, but the PR should be up to date and not require an update as soon as it's merged!

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/3301#pullrequestreview-1067160417
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/3301/review/1067160417 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20220809/5737b487/attachment.htm>