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

<p>This is just a few high level thoughts - the view stuff looks reasonably sane but while I haven't tried to understand the model stuff in detail yet it all looks pretty odd to me.</p><hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r695224113">app/models/communities.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,59 @@
+class Communities
+  require "yaml"
+
+  @local_chapters = {}
+
+  def self.local_chapters(locale)
+    @local_chapters[locale] = local_chapter_for(locale)
</pre>
<p>If that is meant to be memoising the value then you want <code>||=</code> not just <code>=</code>.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r695225312">app/models/communities.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,59 @@
+class Communities
+  require "yaml"
+
+  @local_chapters = {}
+
+  def self.local_chapters(locale)
+    @local_chapters[locale] = local_chapter_for(locale)
+  end
+
+  class << self
</pre>
<p>Why was <code>self.</code> good enough before but now you want to resort to black magic to define these class methods?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r695226266">app/models/communities.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,59 @@
+class Communities
+  require "yaml"
+
+  @local_chapters = {}
+
+  def self.local_chapters(locale)
+    @local_chapters[locale] = local_chapter_for(locale)
+  end
+
+  class << self
+    protected
</pre>
<p>Is this the reason for the black magic? Because you can't have a protected class method so you'd making them instance methods on the metaclass instead? Why do you want protected anyway - is something going to inherit from this?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r695226590">app/models/communities.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,59 @@
+class Communities
+  require "yaml"
+
+  @local_chapters = {}
+
+  def self.local_chapters(locale)
+    @local_chapters[locale] = local_chapter_for(locale)
+  end
+
+  class << self
+    protected
+
+    def local_chapter_for(locale)
+      @local_chapters_index = load_local_chapters
</pre>
<p>See previous comment about memoisation.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r695227982">app/controllers/site_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -106,6 +106,11 @@ def about
     @locale = params[:about_locale] || I18n.locale
   end
 
+  def communities
+    @locale = params[:communities_locale] || I18n.locale
</pre>
<p>Is language the correct way to be selecting a community? Shouldn't it be location driven?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#discussion_r695229084">app/models/communities.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,59 @@
+class Communities
</pre>
<p>Why is this a model? It doesn't seem to act like one... As far as I can see you never actually even create an object of this class... Rather the class methods that look like they should be constructors for the model are actually just returning plain hashes?</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3301#pullrequestreview-737702168">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLIHNLEYACCA4LXXRJLT6QFILANCNFSM5CXUIRLQ">unsubscribe</a>.<br />Triage notifications on the go with GitHub Mobile for <a href="https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675">iOS</a> or <a href="https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email">Android</a>.<img src="https://github.com/notifications/beacon/AAK2OLLWBGTCYHLFRDUWUU3T6QFILA5CNFSM5CXUIRL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOFP4HCGA.gif" height="1" width="1" alt="" /></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-737702168",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/3301#pullrequestreview-737702168",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>