Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring/tobias 117 convert leaflet to js #137

Merged
merged 54 commits into from
Jan 11, 2022
Merged
Show file tree
Hide file tree
Changes from 43 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
d8bf402
Start working on refactoring to js
JulianEgbert Jan 6, 2022
8a0d448
Refactor map elements to js
JulianEgbert Jan 6, 2022
ae9162c
Remove style from head
JulianEgbert Jan 6, 2022
65fae76
removed brackets as suggested
JulianEgbert Jan 6, 2022
a64fed5
Fix variable erroneously dumped to page
laugengebaeck Jan 6, 2022
a60da05
Restore package-lock.json and other feedback
JulianEgbert Jan 6, 2022
c29d2cf
First steps on new js conversion
JulianEgbert Jan 7, 2022
c56ff7c
Refactor routing display
JulianEgbert Jan 7, 2022
38007b1
Add Route to its own layer
JulianEgbert Jan 7, 2022
54b2b66
Trying to fix tests with js
JulianEgbert Jan 7, 2022
4319ac8
Comment on old magic constant
JulianEgbert Jan 7, 2022
c6f9eff
Adapt tests for routing
JulianEgbert Jan 7, 2022
2e9980a
linting stuff
JulianEgbert Jan 7, 2022
2265a8a
Remove test for targets, minor fixes
JulianEgbert Jan 8, 2022
37153ec
[CodeFactor] Apply fixes
code-factor Jan 8, 2022
371eeba
Merge branch 'dev' into refactoring/tobias_117_convert_leaflet_to_js
JulianEgbert Jan 8, 2022
1a82f10
Remove old map from locations, needs more fixing
JulianEgbert Jan 9, 2022
a5ad752
Extra js file for leaflet Map
JulianEgbert Jan 10, 2022
4af5885
prepare locations show
JulianEgbert Jan 10, 2022
75b542c
Remove map from location page
JulianEgbert Jan 10, 2022
543afa2
add debugging help to routing tests
JulianEgbert Jan 10, 2022
776ae89
add waiting time for routing
JulianEgbert Jan 10, 2022
1eab688
remove unnecessary code
JulianEgbert Jan 10, 2022
8ce15e3
adding wait_for_ajax to route tests
JulianEgbert Jan 10, 2022
eda4a02
Merge branch 'dev' into refactoring/tobias_117_convert_leaflet_to_js
JulianEgbert Jan 10, 2022
71a1e7f
Remove old unused code (Commented for target)
JulianEgbert Jan 10, 2022
e717412
tag tests and improve backend routes
JulianEgbert Jan 10, 2022
63ed5ac
improved tagging to new syntax
JulianEgbert Jan 10, 2022
6138956
exclude tests with the tag "local_only"
JulianEgbert Jan 10, 2022
974f469
add jQuery as requested
JulianEgbert Jan 10, 2022
4443c89
code cleanup for jQuery
JulianEgbert Jan 10, 2022
b5f25c6
enable indoor rooms from hg
JulianEgbert Jan 10, 2022
aa4485c
add target marker functionality
JulianEgbert Jan 10, 2022
8777472
remove old variable on index
JulianEgbert Jan 10, 2022
a96f51b
[CodeFactor] Apply fixes
code-factor Jan 10, 2022
4a75617
a try on fixing tests
JulianEgbert Jan 10, 2022
2435fb9
[CodeFactor] Apply fixes
code-factor Jan 10, 2022
3b597a5
fix CodeFactor issue
JulianEgbert Jan 10, 2022
08339e3
improve tests with find and waiting time
JulianEgbert Jan 11, 2022
ee0035b
edit tags for tests
JulianEgbert Jan 11, 2022
98d559e
remove parallel execution in js
JulianEgbert Jan 11, 2022
06517dc
remove wait_for_ajax
JulianEgbert Jan 11, 2022
5a447c4
run route tests only local
JulianEgbert Jan 11, 2022
a3aa0ad
restore yarn.lock
JulianEgbert Jan 11, 2022
fac6fff
add newline to yarn.lock
JulianEgbert Jan 11, 2022
27e8287
Code improvements
JulianEgbert Jan 11, 2022
008eecb
[CodeFactor] Apply fixes
code-factor Jan 11, 2022
048d980
Fix magic string
JulianEgbert Jan 11, 2022
d162876
possible fix for routing tests
JulianEgbert Jan 11, 2022
6705a7b
Parallelize ajax requests
JulianEgbert Jan 11, 2022
3829594
reverse changes for parallel ajax
JulianEgbert Jan 11, 2022
0fdfe85
add comment about race condition
JulianEgbert Jan 11, 2022
6a57af1
Paralize ajax requests WITH tests
JulianEgbert Jan 11, 2022
a0aa143
remove tests for routing (race condition)
JulianEgbert Jan 11, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test_and_deploy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
3 changes: 0 additions & 3 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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)
#
Expand Down
8 changes: 0 additions & 8 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -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:
Expand Down Expand Up @@ -369,7 +362,6 @@ DEPENDENCIES
factory_bot_rails
httparty
jbuilder (~> 2.7)
leaflet-rails!
listen (~> 3.3)
omniauth
omniauth_openid_connect
Expand Down
4 changes: 2 additions & 2 deletions app/assets/constants/buildings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 10 additions & 9 deletions app/assets/constants/locations.rb
Original file line number Diff line number Diff line change
@@ -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
2 changes: 0 additions & 2 deletions app/assets/stylesheets/application.scss
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,6 @@ $fa-font-path: "@fortawesome/fontawesome-free/webfonts";
// CSS partials
@import "components/navbar";

@import "leaflet";

body {
padding: 4rem 0;
}
Expand Down
39 changes: 33 additions & 6 deletions app/controllers/building_map_controller.rb
Original file line number Diff line number Diff line change
@@ -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
16 changes: 2 additions & 14 deletions app/helpers/building_map_helper.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
24 changes: 5 additions & 19 deletions app/helpers/routing_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,17 @@ def self.calculate_route(start, destination)
end

def self.transform_route_to_time_marker(route)
return [] unless 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)
Expand All @@ -60,18 +60,4 @@ def self.transform_route_to_polyline(route)
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: "<img src='/assets/pin.png'>",
class_name: "target-pin"
}
}]
end
end
3 changes: 2 additions & 1 deletion app/javascript/packs/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Dassderdie marked this conversation as resolved.
Show resolved Hide resolved
import "leaflet/dist/leaflet.css";

Rails.start()
Turbolinks.start()
Expand Down
73 changes: 73 additions & 0 deletions app/javascript/packs/building_map.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { displayRoute, setupMap } from './leafletMap.js';

let currentLocation;
const YOUR_LOCATION_MAGIC_STRING = "Your location" // TODO: Change this!!!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because we cannot execute Ruby code inside the JS file, right? It might be difficult to fix (and use the Ruby constant again), because afaik there is no such thing as a .js.erb.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't really pass things from ruby over to js. We might find a workaround, where we provide this thing from the server via an ajax request (like we did with the other constants) but maybe it won't be necessary due to the new UI which uses js as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AJAX just for getting a string constant seems a bit ugly (because we need new/separate endpoints for each constant), but I'm fine with leaving this as it is until we know how the new UI and translation is implemented. We should just keep this in mind.

Copy link
Contributor

@chrisma chrisma Jan 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using an Ajax call to get a string, you could also consider writing the value as a data attribute into an HTML element in the .erb template, which is read by the JS ($('.map').data('location')), i.e. <div class="map" data-location="Your location"></div>. Then the value is bound to the element that requires it. This pattern is sometimes called 'Unobtrusive JS': https://guides.rubyonrails.org/working_with_javascript_in_rails.html#unobtrusive-javascript


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."
);
}
}
);
}
Loading