[openstreetmap/openstreetmap-website] Add microcosms to the website (#2390)
Tom Hughes
notifications at github.com
Sun Oct 13 17:33:18 UTC 2019
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>
More information about the rails-dev
mailing list