From 5527cf0b337762edde2ef7e3e556bcf92c702bfa Mon Sep 17 00:00:00 2001 From: JulianEgbert <57799997+JulianEgbert@users.noreply.github.com> Date: Tue, 11 Jan 2022 14:37:26 +0100 Subject: [PATCH] Refactoring/tobias 117 convert leaflet to js (#137) Refactor leaflet ruby gem to plain js. This merge will remove the map on the location show page, as communicated with team @hpi-swt2/apmw21. We moved functionality from server to client. Coverage will be restored with Issue #149. * Start working on refactoring to js * Refactor map elements to js * Remove style from head * removed brackets as suggested * Fix variable erroneously dumped to page fixes Places::DESTINATIONS variable being dumped into HTML (introduced in #102) * Restore package-lock.json and other feedback * First steps on new js conversion Co-authored-by: TheGrayStone Co-authored-by: Julian Schmidt * Refactor routing display * Add Route to its own layer * Trying to fix tests with js * Comment on old magic constant * Adapt tests for routing * linting stuff * Remove test for targets, minor fixes * [CodeFactor] Apply fixes * Remove old map from locations, needs more fixing * Extra js file for leaflet Map * prepare locations show * Remove map from location page * add debugging help to routing tests * add waiting time for routing * remove unnecessary code * adding wait_for_ajax to route tests * Remove old unused code (Commented for target) * tag tests and improve backend routes * improved tagging to new syntax * exclude tests with the tag "local_only" * add jQuery as requested * code cleanup for jQuery * enable indoor rooms from hg * add target marker functionality * remove old variable on index * [CodeFactor] Apply fixes * a try on fixing tests * [CodeFactor] Apply fixes * fix CodeFactor issue * improve tests with find and waiting time * edit tags for tests * remove parallel execution in js * remove wait_for_ajax * run route tests only local * restore yarn.lock * add newline to yarn.lock * Code improvements * [CodeFactor] Apply fixes * Fix magic string * possible fix for routing tests * Parallelize ajax requests Co-authored-by: Julian Schmidt * reverse changes for parallel ajax * add comment about race condition * Paralize ajax requests WITH tests * remove tests for routing (race condition) Co-authored-by: Lukas Rost Co-authored-by: TheGrayStone Co-authored-by: Julian Schmidt Co-authored-by: codefactor-io --- .github/workflows/test_and_deploy.yml | 2 +- Gemfile | 3 - Gemfile.lock | 8 -- app/assets/constants/buildings.rb | 4 +- app/assets/constants/locations.rb | 19 +-- app/assets/stylesheets/application.scss | 2 - app/controllers/building_map_controller.rb | 39 +++++- app/helpers/building_map_helper.rb | 16 +-- app/helpers/routing_helper.rb | 26 +--- app/javascript/packs/application.js | 3 +- app/javascript/packs/building_map.js | 73 ++++++++++ app/javascript/packs/leafletMap.js | 151 +++++++++++++++++++++ app/views/building_map/index.html.erb | 113 +-------------- app/views/layouts/_navbar.html.erb | 2 +- app/views/locations/show.html.erb | 10 -- config/initializers/leaflet.rb | 3 - config/routes.rb | 4 + spec/features/building_map_spec.rb | 58 ++++++-- 18 files changed, 333 insertions(+), 203 deletions(-) create mode 100644 app/javascript/packs/building_map.js create mode 100644 app/javascript/packs/leafletMap.js delete mode 100644 config/initializers/leaflet.rb diff --git a/.github/workflows/test_and_deploy.yml b/.github/workflows/test_and_deploy.yml index aaf8270b..cc160088 100644 --- a/.github/workflows/test_and_deploy.yml +++ b/.github/workflows/test_and_deploy.yml @@ -52,7 +52,7 @@ jobs: run: | cp config/database.ci.yml config/database.yml bundle exec rails db:setup - bundle exec rspec spec/ --format documentation --format RSpec::Github::Formatter + bundle exec rspec --tag ~local_only spec/ --format documentation --format RSpec::Github::Formatter # Name of the job deploy: diff --git a/Gemfile b/Gemfile index 80170092..a903876e 100644 --- a/Gemfile +++ b/Gemfile @@ -50,9 +50,6 @@ gem 'tod' # Libraries # -# Leaflet for map functionality -gem 'leaflet-rails', git: "git://github.com/Finn-HPI/leaflet-rails.git" - # # Gems that are loaded depending on the environment (development/test/production) # diff --git a/Gemfile.lock b/Gemfile.lock index 7f45ce91..6b4edab6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,10 +1,3 @@ -GIT - remote: git://github.com/Finn-HPI/leaflet-rails.git - revision: 340f65cbc163dcc3523c43592831d43a65381296 - specs: - leaflet-rails (1.7.4) - rails (>= 4.2.0) - GEM remote: https://rubygems.org/ specs: @@ -369,7 +362,6 @@ DEPENDENCIES factory_bot_rails httparty jbuilder (~> 2.7) - leaflet-rails! listen (~> 3.3) omniauth omniauth_openid_connect diff --git a/app/assets/constants/buildings.rb b/app/assets/constants/buildings.rb index fa1eda1a..e208ae4e 100644 --- a/app/assets/constants/buildings.rb +++ b/app/assets/constants/buildings.rb @@ -335,9 +335,9 @@ def self.transform_leaflet_letters(hpi_letters) hpi_letters.map do |hpi_letter| { latlng: hpi_letter[:coordinate], - div_icon: { + divIcon: { html: hpi_letter[:letter], - class_name: "building-icon" + className: "building-icon" } } end diff --git a/app/assets/constants/locations.rb b/app/assets/constants/locations.rb index 508a3553..cd83c7d7 100644 --- a/app/assets/constants/locations.rb +++ b/app/assets/constants/locations.rb @@ -1,11 +1,12 @@ module Locations - def self.transform_leaflet_position(position, name) - { - latlng: position, - div_icon: { - html: name, - class_name: "building-icon" - } - } - end + # Only needed for creating a leaflet marker + # def self.transform_leaflet_position(position, name) + # { + # latlng: position, + # div_icon: { + # html: name, + # class_name: "location-icon" + # } + # } + # end end diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index cf786af0..b6d901ed 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -49,8 +49,6 @@ $fa-font-path: "@fortawesome/fontawesome-free/webfonts"; // CSS partials @import "components/navbar"; -@import "leaflet"; - body { padding: 4rem 0; } diff --git a/app/controllers/building_map_controller.rb b/app/controllers/building_map_controller.rb index a94e7e29..f6ab18c8 100644 --- a/app/controllers/building_map_controller.rb +++ b/app/controllers/building_map_controller.rb @@ -1,11 +1,38 @@ class BuildingMapController < ApplicationController - YOUR_LOCATION_MAGIC_STRING = "Your location".freeze + YOUR_LOCATION_MAGIC_STRING = "Your location".freeze # TODO: This is currenty hard coded in the building_map.js file - def index - @start = RoutingHelper.resolve_coordinates(params[:start]) - @destination = RoutingHelper.resolve_coordinates(params[:dest]) - @route = RoutingHelper.calculate_route(@start, @destination) if @start.present? && @destination.present? + def index; end - @target = params[:target] + def buildings + polygons = BuildingMapHelper.leaflet_polygons + respond_to do |format| + format.json { render json: polygons } + end + end + + def view + view = BuildingMapHelper.leaflet_center + respond_to do |format| + format.json { render json: view } + end + end + + def markers + markers = Buildings.transform_leaflet_letters(Buildings::HPI_LETTERS) + respond_to do |format| + format.json { render json: markers } + end + end + + def route + start = RoutingHelper.resolve_coordinates(params[:start]) + dest = RoutingHelper.resolve_coordinates(params[:dest]) + route = RoutingHelper.calculate_route(start, dest) if start.present? && dest.present? + + result = { polyline: RoutingHelper.transform_route_to_polyline(route), + marker: RoutingHelper.transform_route_to_time_marker(route) } + respond_to do |format| + format.json { render json: result } + end end end diff --git a/app/helpers/building_map_helper.rb b/app/helpers/building_map_helper.rb index fc4fd58b..1733c1e3 100644 --- a/app/helpers/building_map_helper.rb +++ b/app/helpers/building_map_helper.rb @@ -1,9 +1,7 @@ module BuildingMapHelper - def self.leaflet_center(start_coordinates) - center = start_coordinates.nil? ? %w[52.39339 13.13208] : start_coordinates.split(",") - + def self.leaflet_center { - latlng: center, + latlng: %w[52.39339 13.13208], zoom: 17 } end @@ -12,14 +10,4 @@ def self.leaflet_polygons Buildings.transform_leaflet_buildings(Buildings::UNIPOTSDAM_POLYONGS, Buildings::UNIPOTSDAM_STYLING) + Buildings.transform_leaflet_buildings(Buildings::HPI_POLYGONS, Buildings::HPI_STYLING) end - - def self.leaflet_polylines(route) - route.present? ? [RoutingHelper.transform_route_to_polyline(route)] : [] - end - - def self.leaflet_markers(route, target) - Buildings.transform_leaflet_letters(Buildings::HPI_LETTERS) + - RoutingHelper.transform_route_to_time_marker(route) + - RoutingHelper.transform_target_to_marker(target) - end end diff --git a/app/helpers/routing_helper.rb b/app/helpers/routing_helper.rb index 9a0b453d..eaebf4c1 100644 --- a/app/helpers/routing_helper.rb +++ b/app/helpers/routing_helper.rb @@ -39,39 +39,21 @@ def self.calculate_route(start, destination) end def self.transform_route_to_time_marker(route) - return [] unless route - walking_time = format_seconds_as_minsec(route["duration"]) start = route["geometry"]["coordinates"][0] - [{ + { latlng: [start.second, start.first], - div_icon: { + divIcon: { html: walking_time, - class_name: "time-icon" + className: "time-icon" } - }] + } end def self.transform_route_to_polyline(route) - return unless route - coordinates = route["geometry"]["coordinates"].map do |(long, lat)| [lat, long] end { latlngs: coordinates, options: { className: "routing-path" } } end - - def self.transform_target_to_marker(point) - return [] unless point - - coordinates = point.split(",") - - [{ - latlng: coordinates, - div_icon: { - html: "", - class_name: "target-pin" - } - }] - end end diff --git a/app/javascript/packs/application.js b/app/javascript/packs/application.js index 0bf4b717..0777ea25 100644 --- a/app/javascript/packs/application.js +++ b/app/javascript/packs/application.js @@ -18,7 +18,8 @@ import 'bootstrap'; // Fontawesome: https://fontawesome.com/ import "@fortawesome/fontawesome-free/js/all"; -import * as L from "leaflet" +import * as L from "leaflet"; +import "leaflet/dist/leaflet.css"; Rails.start() Turbolinks.start() diff --git a/app/javascript/packs/building_map.js b/app/javascript/packs/building_map.js new file mode 100644 index 00000000..e19c30aa --- /dev/null +++ b/app/javascript/packs/building_map.js @@ -0,0 +1,73 @@ +import { displayRoute, setupMap } from './leafletMap.js'; + +let currentLocation; +const YOUR_LOCATION_MAGIC_STRING = "Your location" // This will be changed when the page supports multiple languages + +setupMap(); + +const start_input_field = $("#start_input")[0]; +start_input_field.addEventListener("change", () => { + request_location(); + if (validate_place_input("start_input", "startOptions")) { + start_input_field.setCustomValidity(""); + } else { + start_input_field.setCustomValidity( + "Please select a valid starting place." + ); + } +}); +start_input_field.dispatchEvent(new Event("change")); + +const dest_input_field = $("#dest_input")[0]; +dest_input_field.addEventListener("change", () => { + if (validate_place_input("dest_input", "destOptions")) { + dest_input_field.setCustomValidity(""); + } else { + dest_input_field.setCustomValidity( + "Please select a valid destination place." + ); + } +}); +dest_input_field.dispatchEvent(new Event("change")); + +$("#navigation_form")[0] + .addEventListener("submit", (event) => { + event.preventDefault(); + if (start_input_field.value === YOUR_LOCATION_MAGIC_STRING) { + start_input_field.value = currentLocation; + } + const start = start_input_field.value; + const dest = dest_input_field.value; + displayRoute(start, dest); + }); + +function validate_place_input(inputId, optionsId) { + const input = $(`#${inputId}`)[0]; + const options = $(`#${optionsId}`)[0].options; + return Array.from(options).some((o) => o.value === input.value); +} + +function request_location() { + if (start_input_field.value !== YOUR_LOCATION_MAGIC_STRING) return; + navigator.geolocation.getCurrentPosition( + (pos) => { + currentLocation = + String(pos.coords.latitude) + + "," + + String(pos.coords.longitude); + start_input_field.setCustomValidity(""); + }, + (error) => { + console.warn(`ERROR(${error.code}): ${error.message}`); + if (error.code === GeolocationPositionError.PERMISSION_DENIED) { + start_input_field.setCustomValidity( + "You have to grant your browser the permission to access your location if you want to use this feature." + ); + } else { + start_input_field.setCustomValidity( + "Your browser could not determine your position. Please choose a different starting place." + ); + } + } + ); +} diff --git a/app/javascript/packs/leafletMap.js b/app/javascript/packs/leafletMap.js new file mode 100644 index 00000000..3b306889 --- /dev/null +++ b/app/javascript/packs/leafletMap.js @@ -0,0 +1,151 @@ +let map; +let routeLayer; + +export async function setupMap() { + map = L.map("map"); + L.tileLayer("http://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png", { + attribution: + '© OpenStreetMap', + maxZoom: 19, + }).addTo(map); + + const [view, buildingPolygons, buildingMarkers] = await Promise.all([ + getView(), + getBuildings(), + getBuildingMarkers(), + ]); + + setView(view); + addTargetMarker(); + addPolygons(buildingPolygons); + addMarkers(buildingMarkers); + + // Add indoor labels + loadGeoJsonFile("assets/lecture-hall-building.geojson"); + map.on("zoomend", recalculateTooltipVisibility); +} + +function addTargetMarker() { + const params = new URLSearchParams(window.location.search); + if (!params.has("target")) return + const target = params.get("target"); + const coordinates = target.split(","); + + const marker = { + latlng: coordinates, + divIcon: { + html: "", + className: "target-pin", + }, + }; + addMarker(marker, map); + setView({latlng: coordinates, zoom: 19}); +} + +export function setView(view) { + map.setView(view["latlng"], view["zoom"]); +} + +export function addPolygons(polygons) { + polygons.forEach((polygon) => { + L.polygon(polygon["latlngs"], polygon["options"]).addTo(map); + }); +} + +export function addMarkers(markers, layer = map) { + markers.forEach((marker) => { + addMarker(marker, layer); + }); +} + +export function addMarker(marker, layer = map) { + marker["divIcon"]["iconSize"] = []; + marker["divIcon"]["iconAnchor"] = [0, 0]; + marker["divIcon"]["popupAnchor"] = [0, 0]; + const icon = L.divIcon(marker["divIcon"]); + L.marker(marker["latlng"], { icon: icon }).addTo(layer); +} + +export function addPolylines(polylines, layer = map) { + polylines.forEach((polyline) => { + addPolyline(polyline, layer); + }); +} + +export function addPolyline(polyline, layer = map) { + L.polyline(polyline["latlngs"], polyline["options"]).addTo(layer); +} + +export async function displayRoute(start, dest) { + const route = await $.ajax({ + type: "GET", + url: "/building_map/route", + data: `start=${start}&dest=${dest}`, + dataType: "json", + }); + if (routeLayer) routeLayer.clearLayers(); + else routeLayer = L.layerGroup(); + addPolyline(route["polyline"], routeLayer); + addMarkers([route["marker"]], routeLayer); + routeLayer.addTo(map); +} + +async function getBuildings() { + return $.ajax({ + type: "GET", + url: "/building_map/buildings", + dataType: "json", + }); +} + +async function getBuildingMarkers() { + return $.ajax({ + type: "GET", + url: "/building_map/markers", + dataType: "json", + }); +} + +async function getView() { + return $.ajax({ + type: "GET", + url: "/building_map/view", + dataType: "json", + }); +} + +function addIndoorLabel(feature, layer) { + layer.bindTooltip(feature.properties.name, { + permanent: true, + direction: "center", + className: "indoor-label", + }); +} + +function loadGeoJsonFile(filename) { + fetch(filename) + .then((response) => response.json()) + .then((geojsonFeatureCollection) => { + // Manually add indoor labels to map + const rooms = L.geoJSON(geojsonFeatureCollection, { + onEachFeature: addIndoorLabel, + }).addTo(map); + rooms.eachLayer((layer) => { + layer.getTooltip().setLatLng(layer.getBounds().getCenter()); + }); + recalculateTooltipVisibility(); + }); +} + +function recalculateTooltipVisibility() { + const zoomLevel = map.getZoom(); + map.eachLayer((layer) => { + if (layer.getTooltip()) { + if (zoomLevel == 19 /* nearest zoom */) { + layer.openTooltip(layer.getBounds().getCenter()); + } else { + layer.closeTooltip(); + } + } + }); +} diff --git a/app/views/building_map/index.html.erb b/app/views/building_map/index.html.erb index a51cf344..5a8edd6f 100644 --- a/app/views/building_map/index.html.erb +++ b/app/views/building_map/index.html.erb @@ -15,12 +15,12 @@
-
- +
@@ -43,14 +43,14 @@ - <%= Places::DESTINATIONS.each do |dest, coords| %> + <% Places::DESTINATIONS.each do |dest, coords| %> <% end %> - <%= Places::DESTINATIONS.each do |dest, coords| %> + <% Places::DESTINATIONS.each do |dest, coords| %> @@ -58,106 +58,7 @@
- <%= map( - center: BuildingMapHelper.leaflet_center(@start), - polygons: BuildingMapHelper.leaflet_polygons, - polylines: BuildingMapHelper.leaflet_polylines(@route), - markers: BuildingMapHelper.leaflet_markers(@route, @target)) - %> +
- - -<%# Indoor labels %> - +<%= javascript_pack_tag 'building_map' %> diff --git a/app/views/layouts/_navbar.html.erb b/app/views/layouts/_navbar.html.erb index 2b1a806d..ccf50d40 100644 --- a/app/views/layouts/_navbar.html.erb +++ b/app/views/layouts/_navbar.html.erb @@ -12,7 +12,7 @@ <% if request.path == building_map_path %>
  • - Navigation + Navigation
    • <% end %> diff --git a/app/views/locations/show.html.erb b/app/views/locations/show.html.erb index 6825fc1b..5e07e18c 100644 --- a/app/views/locations/show.html.erb +++ b/app/views/locations/show.html.erb @@ -22,16 +22,6 @@ -
    - <%= map( - center: { - latlng: [@location.location_latitude, @location.location_longitude], - zoom: 18, - }, - markers: [Locations.transform_leaflet_position([@location.location_latitude, @location.location_longitude], @location.name)]) - %> -
    -

    Opening Times:
    diff --git a/config/initializers/leaflet.rb b/config/initializers/leaflet.rb deleted file mode 100644 index 45363466..00000000 --- a/config/initializers/leaflet.rb +++ /dev/null @@ -1,3 +0,0 @@ -Leaflet.tile_layer = "http://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png" -Leaflet.attribution = '© OpenStreetMap' -Leaflet.max_zoom = 19 diff --git a/config/routes.rb b/config/routes.rb index a92fcf46..cea10897 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -18,6 +18,10 @@ get '/login', to: 'welcome#login' get '/building_map', to: 'building_map#index' + get '/building_map/route', to: 'building_map#route' + get '/building_map/markers', to: 'building_map#markers' + get '/building_map/buildings', to: 'building_map#buildings' + get '/building_map/view', to: 'building_map#view' # '/search_results' get '/search_results', to: 'search_results#index' diff --git a/spec/features/building_map_spec.rb b/spec/features/building_map_spec.rb index 4862fb7e..a54033de 100644 --- a/spec/features/building_map_spec.rb +++ b/spec/features/building_map_spec.rb @@ -28,18 +28,21 @@ it "highlights buildings on the map", js: true do visit building_map_path + page.assert_selector('path.building', count: 15, wait: 5) expect(page).to have_css(".leaflet-interactive") expect(page).to have_selector("path.building", count: 15) end - it "separates HPI and Uni-Potsdam buildings" do + it "separates HPI and Uni-Potsdam buildings", js: true do visit building_map_path + page.assert_selector('path.hpi-building', minimum: 1, wait: 5) expect(page).to have_selector("path.hpi-building", count: 13) expect(page).to have_selector("path.uni-potsdam-building", count: 2) end it "shows the name of the HPI buildings", js: true do visit building_map_path + page.assert_selector("div.building-icon", minimum: 13, wait: 5) expect(page).to have_css(".leaflet-marker-pane") expect(page).to have_css(".leaflet-marker-icon") expect(page).to have_selector("div.building-icon", minimum: 13) @@ -47,31 +50,56 @@ it "shows the pin of a target point", js: true do visit building_map_path(target: "52.393913,13.133082") - expect(page).to have_css('.target-pin') + find(".target-pin", wait: 5) + expect(page).to have_css(".target-pin") end - context "with route" do - it "shows a calculated route", js: true do - visit building_map_path(start: "52.393913,13.133082", dest: "52.393861,13.129606") - expect(page).to have_css(".routing-path") - end + it "shows no route, if it's not requested", js: true do + visit building_map_path + expect(page).not_to have_css(".routing-path") + expect(page).not_to have_css(".time-icon") + end + + it "renders the map with all special features", js: true do + visit building_map_path + expect(page).to have_css("#map") + expect(page).to have_css(".leaflet-container") + page.assert_selector("path.building", count: 15, wait: 5) + expect(page).to have_selector("path.hpi-building", count: 13) + expect(page).to have_selector("div.building-icon", minimum: 13) + expect(page).to have_selector("path.uni-potsdam-building", count: 2) + end - it "shows no route, if it's not requested", js: true do + # Following tests might be inconsistent when run on GitHub Actions. + context "with route", inconsistent: true, local_only: true do + before do visit building_map_path - expect(page).not_to have_css(".routing-path") - expect(page).not_to have_css(".time-icon") + find("#nav-link-navigation").click + fill_in 'start', with: 'Haus A' + fill_in 'dest', with: 'Haus L' + click_on 'Go' end - it "shows no route, if not all necessary parameters are provided", js: true do - visit building_map_path(start: "52.393913,13.133082") - expect(page).not_to have_css(".routing-path") - expect(page).not_to have_css(".time-icon") + it "shows a calculated route", js: true do + find(".routing-path", wait: 15) + expect(page).to have_css(".routing-path") end it "shows the time of a calculated route", js: true do - visit building_map_path(start: "52.393913,13.133082", dest: "52.393861,13.129606") + find(".time-icon", wait: 15) expect(page).to have_css(".time-icon") end + + it "only shows one route at the time", js: true do + find(".routing-path", wait: 15) + expect(page).to have_css(".routing-path", count: 1) + fill_in 'start', with: 'Haus A' + fill_in 'dest', with: 'Haus L' + click_on 'Go' + find(".routing-path", wait: 5) + expect(page).to have_css(".routing-path", count: 1) + end + end end end