<p></p>
<p><b>@tomhughes</b> requested changes on this pull request.</p>

<p dir="auto">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...</p><hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r941647082">lib/osm_community_index.rb</a>:</p>
<pre style='color:#555'>> @@ -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/*"))
</pre>
<p dir="auto">I would make that match <code class="notranslate">*.yaml</code> as an extra protection.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r941648918">lib/osm_community_index.rb</a>:</p>
<pre style='color:#555'>> @@ -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("_", "-")
</pre>
<p dir="auto">Presumably this affects more than just <code class="notranslate">en-GB</code> so maybe the comment should be more generic, like <code class="notranslate">language-COUNTRY</code> or something?</p>
<p dir="auto">Are we sure this is the only mismatch? Has anybody done a complete cross check between our language list and the OCI one?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r941652151">lib/osm_community_index.rb</a>:</p>
<pre style='color:#555'>> +    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"]
</pre>
<p dir="auto">We should be consistent in whether we index <code class="notranslate">community</code> using symbols or strings - we use strings here but a symbol a few lines above.</p>
<p dir="auto">In fact given community is a model can't we use member syntax? So <code class="notranslate">community.id</code> and <code class="notranslate">community.strings</code>?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r941652996">lib/osm_community_index.rb</a>:</p>
<pre style='color:#555'>> @@ -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|
</pre>
<p dir="auto">I'd use <code class="notranslate">each_with_object</code> here rather than initialising <code class="notranslate">data</code> before and then using it in the loop.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r941654114">lib/osm_community_index.rb</a>:</p>
<pre style='color:#555'>> +    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 } } })
</pre>
<p dir="auto">Does this actually need the outermost <code class="notranslate">{}</code> around the hash?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r941662082">lib/osm_community_index.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,26 @@
+module OsmCommunityIndex
+  def self.add_to_i18n
+    communities = Community.all
</pre>
<p dir="auto">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...</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r941645113">package.json</a>:</p>
<pre style='color:#555'>> @@ -7,6 +7,7 @@
     "js-cookie": "^3.0.0",
     "leaflet": "^1.6.0",
     "leaflet.locatecontrol": "^0.75.0",
+    "osm-community-index": "^5.1.3",
</pre>
<p dir="auto">Well yes, but the PR should be up to date and not require an update as soon as it's merged!</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#pullrequestreview-1067160417">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLIL4CUW65ZAI67D6R3VYKOI7ANCNFSM5CXUIRLQ">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLKIKT76GM44EF2ZQPLVYKOI7A5CNFSM5CXUIRL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOH6NZGYI.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/3301/review/1067160417</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/3301#pullrequestreview-1067160417",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/3301#pullrequestreview-1067160417",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>