Endebert / squadmc

Map-based mortar calculator for Squad
https://squadmc.ende.pro
MIT License
121 stars 36 forks source link

Implemented vue-router for maps #94

Closed ekzyis closed 3 years ago

ekzyis commented 4 years ago

Close https://github.com/Endebert/squadmc/issues/12

Hey, I finally came around to do what I said I will do over 2 years ago :D

In https://github.com/Endebert/squadmc/issues/12, you mentioned the following:

Regarding implementation, you would need a mapping between the path name and the map, so best approach is probably adding a routerName property to each map in MAPDATA.

I did this without adding a routerName property but instead just mapped the map names from the MapData#getMapNames function onto routes and added a property initialMap to Map.vue. So when a user goes to route /Kohat, the Map component is initialized with initialMap set to Kohat which will then load the Kohat map.

When using the dropdown to change the map, I push a new location to the router object which makes the browser buttons available to go back to the previous map.

I am currently not entire sure if this leads to duplicate loading issues because the router push should already handle the map change because the Map component will be reloaded so the code after the location push should not be needed but as far as I could see, this worked with the least amount of changes without any problems :thinking:

ekzyis commented 4 years ago

Sorry, I am currently quite busy with my bachelor thesis but I will come back to you this weekend!

ekzyis commented 4 years ago

Okay, I implemented your changes.

I just have one thing which I did not want to do without asking first since it would lead to a quite large diff (even though it's just moving code around):

The previous system design, which executed the function changeMap to change the map and save the now selected map in a variable, basically works against the router. Since I push a new location route in changeMap:

/**
 * Changes current map to the given map
 * @param newMap map to show
 */
changeMap(newMap) {
  console.log("changeMap:", newMap);
  this.$router.push(newMap).catch((err) => {
    // Ignore the vuex err regarding navigating to the page they are already on.
    if (err.name === "NavigationDuplicated" && err.message
    && err.message.includes("Avoided redundant navigation to current location")) {
      return;
    }
    // But print any other errors to the console
    console.error(err);
  });
  const squadMap = this.mapData.getSquadMap(newMap);
  // ...

all the code that comes afterwards is unnecessary because the component will remount anyway.

I think the solution would be to move all that code in changeMap into mounted.

The cleaning code can be removed completely because the component will be remounted as a whole new instance:

// clear map completely
this.map.eachLayer((layer) => {
  this.map.removeLayer(layer);
});

// clear map related objects
this.mortar = undefined;
this.target = undefined;
this.secondaryTarget = undefined;
this.placedTargets = [];
this.placedMortars = [];
this.placedFobs = [];

And one can remove this else because changeMap will then do nothing else than pushing a new route. This was also the issue why that "duplicate navigation" error showed up in the first place.

// set selected map, defined already if loaded from localStorage
    if (!this.selectedMap || this.maps.indexOf(this.selectedMap) === -1) {
      this.selectedMap = this.maps[0];
    } else {
      // since selectedMap is already defined and doesn't trigger changeMap, we do it here manually
      this.changeMap(this.selectedMap);
    }

--- UPDATE

Okay, I have implemented those changes suggested by me in this commit without waiting for your approval :D https://github.com/Endebert/squadmc/pull/94/commits/ee9a6ceccfe651bd2b9a2a502aabf8f5f2d34054

This way, you can immediately try out what I mean and see if you like it and everything still works :) If not, we can just revert that commit :+1:

ekzyis commented 3 years ago

@Endebert any updates on this? Just a friendly reminder if you may just have forgotten about this here :)

ekzyis commented 3 years ago

Okay, no problem, let me know if any issues come up :) And also thanks for your work on this tool!