<p><b>@tomhughes</b> requested changes on this pull request.</p>
<p>This is just a quick first pass through some obvious issues rather than a detailed review.</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334287278">app/assets/javascripts/event_attendances.coffee</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,3 @@
+# Place all the behaviors and hooks related to the matching controller here.
+# All this logic will automatically be available in application.js.
+# You can use CoffeeScript in this file: http://coffeescript.org/
</pre>
<p>This, and a whole load of other empty files that have been added automatically by a generator, should be removed as they're not actually being used. Especially since we don't use coffescript!</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334287299">app/helpers/event_attendances_helper.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,2 @@
+module EventAttendancesHelper
+end
</pre>
<p>Empty helpers can go as well.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334287338">app/models/application_record.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,4 @@
+# Base ApplicationRecord Class
+class ApplicationRecord < ActiveRecord::Base
+ self.abstract_class = true
+end
</pre>
<p>We don't currently use <code>ApplicationRecord</code> so please don't introduce it like this - just make your models inherit in the same way as existing ones.</p>
<p>If you want to introduce <code>ApplicationRecord</code> then we can discuss that but it should be done as a separate task and introduced globally for consistency.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334287367">app/views/events/show.html.erb</a>:</p>
<pre style='color:#555'>> + .when { color: #777; font-size: 120%; }
+ .attendees > a
+ {
+ display: block;
+ width: 8em;
+ height: 8em;
+ border: 1px solid #888;
+ border-radius: 4px;
+ float: left;
+ margin: 1em;
+ padding: 1em;
+ background-color: rgb(238, 238, 238);
+ box-sizing: border-box;
+ text-decoration: none;
+ }
+</style>
</pre>
<p>Inline CSS is not allowed so this needs to be moved to a stylesheet.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334287392">app/views/events/show.html.erb</a>:</p>
<pre style='color:#555'>> + <% end %>
+ </p>
+ </div>
+ <div style="width: 400px">
+ <p>
+ Location: <strong><a href="https://www.openstreetmap.org/node/6125774816"><%= @event.location %></a></strong>
+ </p>
+ <a href="https://www.openstreetmap.org/directions?engine=fossgis_osrm_car&route=44.97067%2C-93.26822%3B38.90201%2C-77.04469#map=6/42.026/-85.156">Directions to this location.</a>
+ <%= image_tag("dc_library.png", size: "400x400") %>
+ </div>
+</div>
+
+
+
+
+<!--<%#= link_to 'Edit', edit_event_path(@event) %> |-->
</pre>
<p>Don't just comment things out - if you don't need them then remove them.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334287403">app/views/microcosms/_form.html.erb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,70 @@
+<%# if microcosm.errors.any? %>
+<!-- <div id="error_explanation">-->
+<!-- <h2><%#= pluralize(microcosm.errors.count, "error") %> prohibited this microcosm from being saved:</h2>-->
+<!-- <ul>-->
+ <%# microcosm.errors.full_messages.each do |message| %>
+<!-- <li><%#= message %></li>-->
+ <%# end %>
+<!-- </ul>-->
+<!-- </div>-->
+<%# end %>
</pre>
<p>Commented out code again.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334287430">app/views/microcosms/_form.html.erb</a>:</p>
<pre style='color:#555'>> + <%= form.text_field :location, id: :microcosm_location %>
+ </div>
+
+ <div class="field">
+ <%= form.label :lat %>
+ <%= form.text_field :lat, id: :microcosm_lat %> (decimal)
+ </div>
+
+ <div class="field">
+ <%= form.label :lon %>
+ <%= form.text_field :lon, id: :microcosm_lon %> (decimal)
+ </div>
+
+ <p>
+ To get a bounding box use Geofabrik's <a href="http://tools.geofabrik.de/calc/#type=geofabrik_standard&bbox=-120.848658,31.674368,-79.375023,49.413863&tab=1&proj=EPSG:4326&places=2">Tile Calculator</a> tool.
+ </p>
</pre>
<p>I don't know what the solution is offhand but I don't think we want to be referring people to a third party service like this.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334287442">app/views/microcosms/show.html.erb</a>:</p>
<pre style='color:#555'>> + .mic_details ul > li { display: inline-block; }
+ .members > a
+ {
+ display: block;
+ width: 8em;
+ height: 8em;
+ border: 1px solid #888;
+ border-radius: 4px;
+ float: left;
+ margin: 1em;
+ padding: 1em;
+ background-color: rgb(238, 238, 238);
+ box-sizing: border-box;
+ text-decoration: none;
+ }
+</style>
</pre>
<p>More inline styles.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334287535">db/migrate/20190901143302_create_friendly_id_slugs.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,21 @@
+MIGRATION_CLASS =
+ if ActiveRecord::VERSION::MAJOR >= 5
+ ActiveRecord::Migration["#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}"]
+ else
+ ActiveRecord::Migration
+ end
</pre>
<p>Why on earth is this needed - we know what version of <code>ActiveRecord</code> we are starting from so there is no need for all this noise. I'm guessing it's automatically generated code, but we have no need for it.</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/2390?email_source=notifications&email_token=AAK2OLJHTFGK733FZWVBLLTQONLV5A5CNFSM4JAHYW42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHY4NIA#pullrequestreview-301057696">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLOUCDNHAGCABTSCOALQONLV5ANCNFSM4JAHYW4Q">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AAK2OLL52GSXMB3A5RXODX3QONLV5A5CNFSM4JAHYW42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHY4NIA.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/2390?email_source=notifications\u0026email_token=AAK2OLJHTFGK733FZWVBLLTQONLV5A5CNFSM4JAHYW42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHY4NIA#pullrequestreview-301057696",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/2390?email_source=notifications\u0026email_token=AAK2OLJHTFGK733FZWVBLLTQONLV5A5CNFSM4JAHYW42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHY4NIA#pullrequestreview-301057696",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>