Azgaar / Fantasy-Map-Generator

Web application generating interactive and highly customizable maps
https://azgaar.github.io/Fantasy-Map-Generator
Other
4.48k stars 636 forks source link

Nautical mile option in Units-Editor #1059

Closed Avengium closed 5 months ago

Avengium commented 5 months ago

Description

I added nautical mile as an option for Units Editor. In Distance --> Distance Unit.

Type of change

New option in dropdown distance unit.

Versioning

netlify[bot] commented 5 months ago

Deploy Preview for afmg ready!

Name Link
Latest commit d2bf08041cfc14afba47580270f70b0854857954
Latest deploy log https://app.netlify.com/sites/afmg/deploys/66031959ba4fd00008df66ef
Deploy Preview https://deploy-preview-1059--afmg.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Avengium commented 5 months ago

The "league" unit in distance has the numbers of a nautical league. If Azgaar want to clarify the current one, already on the program has the numbers of a nautical league, the text could be updated to:

    function toKilometer(v) {
      if (unit === "km") return v;
      else if (unit === "mi") return v * 1.60934;
      else if (unit === "lg") return v * 4.828;
      else if (unit === "vr") return v * 1.0668;
      else if (unit === "nmi") return v * 1.852;
      else if (unit === "nlg") return v * 5.556;
      return 0; // 0 if distanceUnitInput is a custom unit
    }

and the HTML like:

          <div data-tip="Select a distance unit or provide a custom name">
            <div>Distance unit:</div>
            <select id="distanceUnitInput" data-stored="distanceUnit">
              <option value="mi" selected>Mile (mi)</option>
              <option value="km">Kilometer (km)</option>
              <option value="lg">League (lg)</option>
              <option value="vr">Versta (vr)</option>
              <option value="nmi">Nautical mile (nmi)</option>
              <option value="nlg">Nautical league (nlg)</option>
              <option value="custom_name">Custom name</option>
            </select>
          </div>
StempunkDev commented 5 months ago

The "league" unit in distance has the numbers of a nautical league. If Azgaar want to clarify the current one, already on the program has the numbers of a nautical league, the text could be updated to:

    function toKilometer(v) {
      if (unit === "km") return v;
      else if (unit === "mi") return v * 1.60934;
      else if (unit === "lg") return v * 4.828;
      else if (unit === "vr") return v * 1.0668;
      else if (unit === "nmi") return v * 1.852;
      else if (unit === "nlg") return v * 5.556;
      return 0; // 0 if distanceUnitInput is a custom unit
    }

I would like to suggest the following refactor of the toKilometer Function for possibile expansions in the future :

    function toKilometer(v) {
      switch(unit) {
      case 'km': return v;
      case 'mi': return v * 1.60934;
      case 'lg': return v * 4.828;
      case 'vr': return v * 1.0668;
      case 'nmi': return v * 1.852;
      case 'nlg': return v * 5.556;
      default: return 0; // 0 if distanceUnitInput is a custom unit
     }
    }

No break; is needed as we return in every line. I would say this is easier to read.

Azgaar commented 5 months ago

I would like to suggest the following refactor of the toKilometer Function for possibile expansions in the future

I don't see how it's better. If with earlier exit can be simpler than switch.

Avengium commented 5 months ago

Azgaar, what's your decision on this added code? Current commit only has nautical mile and proposed edits on comments are:

Also, do we put the switch or the list of else ifs?

Azgaar commented 5 months ago

It's fine, you can add. The code should be like that:

    function toKilometer(v) {
      if (unit === "km") return v;
      if (unit === "mi") return v * 1.60934;
      if (unit === "lg") return v * 5.556;
      if (unit === "vr") return v * 1.0668;
      return 0;
    }

Else is not required as we are returning value immediately.

Azgaar commented 5 months ago

I'm ok to merge, please update the version

Azgaar commented 5 months ago

@Avengium, hm, not that way. Now there is a conflict. Also need to update hash for changed scripts in index.html.