Refactoring/tobias 117 convert leaflet to js #137

JulianEgbert · 2022-01-06T10:08:15Z

We removed the ruby gem for leaflet and do the map with plain js.

JulianEgbert · 2022-01-06T10:11:06Z

@TheGrayStone: There is a method called transform_target_to_marker that we are uncertain about right now. We made some changes to the map and just want to make sure, that your feature is still working because we weren't able to test it ourself. If there is a test for it, it should be fine but we just want to double check this.

codecov · 2022-01-06T10:36:56Z

Codecov Report

Merging #137 (a0aa143) into dev (db260ac) will decrease coverage by 2.93%.
The diff coverage is 73.91%.

@@            Coverage Diff             @@
##              dev     #137      +/-   ##
==========================================
- Coverage   98.52%   95.58%   -2.94%     
==========================================
  Files          25       25              
  Lines         406      408       +2     
==========================================
- Hits          400      390      -10     
- Misses          6       18      +12

Impacted Files	Coverage Δ
app/assets/constants/buildings.rb	`100.00% <ø> (ø)`
app/assets/constants/locations.rb	`100.00% <ø> (ø)`
app/helpers/routing_helper.rb	`80.00% <ø> (-20.00%)`	⬇️
app/controllers/building_map_controller.rb	`72.72% <71.42%> (-27.28%)`	⬇️
app/helpers/building_map_helper.rb	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db260ac...a0aa143. Read the comment docs.

rdunker

🥳

app/views/building_map/index.html.erb

JulianEgbert · 2022-01-06T12:39:33Z

Troubleshooting
We encountered problems when using a new node version so we started collecting these kind of issues in a wiki page: https://github.com/hpi-swt2/compass-portal-blue/wiki/Known-Issues

niklasmohrin · 2022-01-06T15:01:40Z

@TheGrayStone told me earlier that their team is also working on something similar, let's refrain from merging and putting too much additional work into this before they clarify here @hpi-swt2/rh21

fixes Places::DESTINATIONS variable being dumped into HTML (introduced in #102)

app/views/building_map/index.html.erb

app/javascript/packs/application.js

config/initializers/leaflet.rb

app/views/building_map/index.html.erb

Co-authored-by: TheGrayStone <[email protected]> Co-authored-by: Julian Schmidt <[email protected]>

Outdated

chrisma · 2022-01-10T22:52:09Z

Thanks for the investigation! Could the following still help, perhaps?

find(".active")
execute_script("$('.active').focus()")
Capybara will wait until a matching element is on the page, and then dispatch a JavaScript command which interacts with it.

chrisma · 2022-01-10T22:56:19Z

The find method also takes a wait parameter to increase the default wait time, in case that might be needed: https://stackoverflow.com/questions/56837464/capybara-wait-until-button-is-enabled

JulianEgbert · 2022-01-11T11:14:02Z

The find method also takes a wait parameter to increase the default wait time, in case that might be needed: https://stackoverflow.com/questions/56837464/capybara-wait-until-button-is-enabled

I tested this with page.assert_selector("path.building", count: 15, wait: 5) for cases where we have muttiple results so find won't work (see here). It seems to be a bit better but the tests are still inconsistent.

Some tests (that then fail, like Building Map page map highlights buildings on the map throw the following error:

[0111/121442.075:INFO:CONSOLE(26188)] "Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'min')", source: http://127.0.0.1:62875/packs-test/js/application-bdd1e7e7bddd913eb135.js (26188)

although we never check for a property called "min".
And other times they also throw this error but pass the test.

This is the referenced js file:
jsFile.txt

Fixed this!
Problem seems to be the parallel execution via .then(...). Now we use await and it seems to work, at least for tests without routes.

JulianEgbert · 2022-01-11T12:43:17Z

Coverage will increase with Issue #149, where we fix the tests that are currently only run locally (the features however work).

postmartem

Seems good

Dassderdie · 2022-01-11T13:03:42Z

Fixed this! Problem seems to be the parallel execution via .then(...). Now we use await and it seems to work, at least for tests without routes.

I could imagine that there still is a race condition and it just doesn't appear by chance in the current tests.

You changed in 98d559e

// these operations are executed in parallel
getView().then((view) => {
    setView(view);
    addTargetMarker();
});

getBuildings().then((buildingPolygons) => {
    addPolygons(buildingPolygons);
});

getBuildingMarkers().then((buildingMarkers) => {
    addMarkers(buildingMarkers);
});

to

const view = await getView();
setView(view);
addTargetMarker();

const buildingPolygons = await getBuildings();
addPolygons(buildingPolygons);

const buildingMarkers = await getBuildingMarkers();
addMarkers(buildingMarkers);

From my understanding this should be equivalent to (except rejections error handling):

getView().then((view) => {
    setView(view);
    addTargetMarker();
    getBuildings().then((buildingPolygons) => {
        addPolygons(buildingPolygons);
        getBuildingMarkers().then((buildingMarkers) => {
            addMarkers(buildingMarkers);
        });
    });
});

I don't see how this could have fixed the errors...

laugengebaeck

Looks good 😄 We will have to change lots of code in the other pull requests (because they stiil use JS inside the .html.erb file), but I'm glad this finally works!

laugengebaeck · 2022-01-11T13:03:43Z

app/javascript/packs/building_map.js

+import { displayRoute, setupMap } from './leafletMap.js';
+
+let currentLocation;
+const YOUR_LOCATION_MAGIC_STRING = "Your location" // TODO: Change this!!!


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.

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?

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.

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

Co-authored-by: Julian Schmidt

Dassderdie · 2022-01-11T13:38:08Z

It seemed like there was a race condition. We resolved it by skipping the respective tests for now.

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 <[email protected]> Co-authored-by: Julian Schmidt <[email protected]> * 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 <[email protected]> Co-authored-by: TheGrayStone <[email protected]> Co-authored-by: Julian Schmidt <[email protected]> Co-authored-by: codefactor-io <[email protected]>

JulianEgbert added 2 commits January 6, 2022 10:19

Start working on refactoring to js

d8bf402

Refactor map elements to js

8a0d448

JulianEgbert added the T.O.B.I.A.S. (TF1 & TF2) label Jan 6, 2022

JulianEgbert requested a review from TheGrayStone January 6, 2022 10:11

JulianEgbert assigned JulianEgbert and Finn-HPI Jan 6, 2022

JulianEgbert linked an issue Jan 6, 2022 that may be closed by this pull request

Convert Leaflet to JS #117

Closed

JulianEgbert requested a review from Dassderdie January 6, 2022 10:29

Remove style from head

ae9162c

rdunker previously approved these changes Jan 6, 2022

View reviewed changes

app/views/building_map/index.html.erb Outdated Show resolved Hide resolved

removed brackets as suggested

65fae76

Fix variable erroneously dumped to page

a64fed5

fixes Places::DESTINATIONS variable being dumped into HTML (introduced in #102)

Dassderdie requested changes Jan 6, 2022

View reviewed changes

app/views/building_map/index.html.erb Outdated Show resolved Hide resolved

app/javascript/packs/application.js Show resolved Hide resolved

config/initializers/leaflet.rb Show resolved Hide resolved

app/views/building_map/index.html.erb Outdated Show resolved Hide resolved

Restore package-lock.json and other feedback

a60da05

niklasmohrin mentioned this pull request Jan 6, 2022

Encountered Errors #141

Closed

JulianEgbert and others added 7 commits January 7, 2022 12:02

First steps on new js conversion

c29d2cf

Co-authored-by: TheGrayStone <[email protected]> Co-authored-by: Julian Schmidt <[email protected]>

Refactor routing display

c56ff7c

Add Route to its own layer

38007b1

Trying to fix tests with js

54b2b66

Comment on old magic constant

4319ac8

Adapt tests for routing

c6f9eff

linting stuff

2e9980a

JulianEgbert requested a review from Dassderdie January 8, 2022 11:31

fix CodeFactor issue

3b597a5

JulianEgbert added 5 commits January 11, 2022 12:14

improve tests with find and waiting time

08339e3

edit tags for tests

ee0035b

remove parallel execution in js

98d559e

remove wait_for_ajax

06517dc

run route tests only local

5a447c4

JulianEgbert and others added 4 commits January 11, 2022 13:48

restore yarn.lock

a3aa0ad

add newline to yarn.lock

fac6fff

Code improvements

27e8287

[CodeFactor] Apply fixes

008eecb

postmartem approved these changes Jan 11, 2022

View reviewed changes

Fix magic string

048d980

possible fix for routing tests

d162876

laugengebaeck approved these changes Jan 11, 2022

View reviewed changes

JulianEgbert added 3 commits January 11, 2022 14:13

Parallelize ajax requests

6705a7b

Co-authored-by: Julian Schmidt

reverse changes for parallel ajax

3829594

add comment about race condition

0fdfe85

Dassderdie approved these changes Jan 11, 2022

View reviewed changes

JulianEgbert added 2 commits January 11, 2022 14:30

Paralize ajax requests WITH tests

6a57af1

remove tests for routing (race condition)

a0aa143

JulianEgbert merged commit 7ade7ad into dev Jan 11, 2022

JulianEgbert deleted the refactoring/tobias_117_convert_leaflet_to_js branch January 11, 2022 13:37

laugengebaeck mentioned this pull request Jan 13, 2022

Feature/tobias 112 navigation markers #138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring/tobias 117 convert leaflet to js #137

Refactoring/tobias 117 convert leaflet to js #137

JulianEgbert commented Jan 6, 2022

JulianEgbert commented Jan 6, 2022

codecov bot commented Jan 6, 2022 •

edited

Loading

rdunker left a comment

JulianEgbert commented Jan 6, 2022 •

edited

Loading

niklasmohrin commented Jan 6, 2022

chrisma commented Jan 10, 2022

chrisma commented Jan 10, 2022

JulianEgbert commented Jan 11, 2022 •

edited

Loading

JulianEgbert commented Jan 11, 2022

postmartem left a comment

Dassderdie commented Jan 11, 2022

laugengebaeck left a comment

laugengebaeck Jan 11, 2022

JulianEgbert Jan 11, 2022

laugengebaeck Jan 11, 2022

chrisma Jan 11, 2022 •

edited

Loading

Dassderdie commented Jan 11, 2022

Refactoring/tobias 117 convert leaflet to js #137

Refactoring/tobias 117 convert leaflet to js #137

Conversation

JulianEgbert commented Jan 6, 2022

JulianEgbert commented Jan 6, 2022

codecov bot commented Jan 6, 2022 • edited Loading

Codecov Report

rdunker left a comment

Choose a reason for hiding this comment

JulianEgbert commented Jan 6, 2022 • edited Loading

niklasmohrin commented Jan 6, 2022

chrisma commented Jan 10, 2022

chrisma commented Jan 10, 2022

JulianEgbert commented Jan 11, 2022 • edited Loading

JulianEgbert commented Jan 11, 2022

postmartem left a comment

Choose a reason for hiding this comment

Dassderdie commented Jan 11, 2022

laugengebaeck left a comment

Choose a reason for hiding this comment

laugengebaeck Jan 11, 2022

Choose a reason for hiding this comment

JulianEgbert Jan 11, 2022

Choose a reason for hiding this comment

laugengebaeck Jan 11, 2022

Choose a reason for hiding this comment

chrisma Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Dassderdie commented Jan 11, 2022

codecov bot commented Jan 6, 2022 •

edited

Loading

JulianEgbert commented Jan 6, 2022 •

edited

Loading

JulianEgbert commented Jan 11, 2022 •

edited

Loading

chrisma Jan 11, 2022 •

edited

Loading