[openstreetmap/openstreetmap-website] Add microcosms to the website (#2390)

[Tom Hughes]

tomhughes requested changes on this pull request.

This is just a quick first pass through some obvious issues rather than a detailed review.

> @@ -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/

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!

> @@ -0,0 +1,2 @@
+module EventAttendancesHelper
+end

Empty helpers can go as well.

> @@ -0,0 +1,4 @@
+# Base ApplicationRecord Class
+class ApplicationRecord < ActiveRecord::Base
+  self.abstract_class = true
+end

We don't currently use `ApplicationRecord` so please don't introduce it like this - just make your models inherit in the same way as existing ones.

If you want to introduce `ApplicationRecord` then we can discuss that but it should be done as a separate task and introduced globally for consistency.

> +  .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>

Inline CSS is not allowed so this needs to be moved to a stylesheet.

> +      <% 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) %> |-->

Don't just comment things out - if you don't need them then remove them.

> @@ -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 %>

Commented out code again.

> +    <%= 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>

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.

> +  .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>

More inline styles.

> @@ -0,0 +1,21 @@
+MIGRATION_CLASS =
+  if ActiveRecord::VERSION::MAJOR >= 5
+    ActiveRecord::Migration["#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}"]
+  else
+    ActiveRecord::Migration
+  end

Why on earth is this needed - we know what version of `ActiveRecord` 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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/2390#pullrequestreview-301057696
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20191013/6ae89bc1/attachment-0001.html>