Feature request: separate outline settings for text information only

thw0rted commented 5 years ago

I'm using uniqueDesignation to add text beside a symbol, and in order to make it legible against varied backgrounds I need it to render as black with a white outline. I saw #50 and have been using outlineWidth successfully to achieve this effect. Unfortunately, this also applies the same outline to the icon itself, and in my case this actually has the effect of reducing legibility of certain symbology decorations against certain backgrounds. I'd like to see a way to independently control the outline so that "text" is outlined while "symbols" are not.

spatialillusions commented 5 years ago

If you go to: https://github.com/spatialillusions/milsymbol/blob/master/src/symbolfunctions/textfields.js#L907

and update it to:

    if (this.style.outlineWidth > 0 || this.style.textOutlineWidth > 0)
      drawArray1.push(
        ms.outline(
          drawArray2,
          this.style.outlineWidth || this.style.textOutlineWidth,
          this.style.strokeWidth,
          typeof this.style.outlineColor === "object"
            ? this.style.outlineColor[this.metadata.affiliation]
            : this.style.outlineColor
        )
      );

and add this.style.textOutlineWidth = 0 here https://github.com/spatialillusions/milsymbol/blob/master/src/ms/symbol.js#L71

and then set the option textOutlineWidth when you initiate your symbol then it should solve things.

If you would update the documentation as well I would of course welcome a pull request for this.

thw0rted commented 5 years ago

I found that bit last night just as I was closing up to go home, but this certainly will make it simple for me. I'll need a little time to set up a dev environment to test it properly but hopefully I can get it put together at some point today.

thw0rted commented 5 years ago

I've had a couple issues getting the build up and running.

CONTRIBUTING.md says to run the build-dev script but that doesn't exist
The only VSCode launcher defined is for Jest tests but you use tead now
When I try to run tead it immediately fails with the error [...\node_modules\tead\src\index.js:1 TypeError: Hu.wrap is not a function], and the only thing I can think of is that maybe something broke in a very recent version of Node because it hasn't been updated in a year? It's not in your code, because it also fails the same way when run from its own directory under node_modules.

I'll keep poking at it but I want to try to get a known-good state before I make any changes.

spatialillusions commented 5 years ago

You don't need to run build-dev anymore, so just build should do the trick. This should be fixed in the documentation. Point two need to be fixed, missed to update when switched to tead. Need to look into why tead fails, hopefully it can be fixed, otherwise I will have to migrate the tests once again...

thw0rted commented 5 years ago

I definitely traced the exception to the included esm dependency in tead, but since they include it as a submodule instead of a proper npm dependency and didn't elect to include source maps, I'm stumped from there. I don't see related issues on the tead issue tracker so maybe it's something unique to my machine? It's failing with a simple clone then test, though, so it'd have to be specific to my environment (Windows, node 12.7.0).

For now, I guess I'll just make the changes and test them out in my own consuming application.

thw0rted commented 5 years ago

My first pass is here. I allowed a value of false to mean "fall back on outlineWidth" because otherwise it's not possible to say "don't outline text, but do outline everything else". I tried to follow your style and used a separate commit to remove the reference to build-dev from the docs. I still can't run tests but I used npm link to include these changes in my own project and it seems to work from there.

ETA: I can open a PR if you like but thought maybe it would be best to do that after tests are working again? I didn't write any tests for the new feature...

spatialillusions commented 5 years ago

I have verified that there is an error with tead when I updated Node from v8.9.4 to v10.16.3. I have contacted the maintainer of tead to see if he is planning to fix this, otherwise I will have to look into other solutions.

spatialillusions commented 5 years ago

Fixed the problems with the test in https://github.com/spatialillusions/milsymbol/commit/491cb14be33505128bfec2c917fbb2f6f2b5d1f9. I have now updated all dependencies and made sure that they work with node 10.

thw0rted commented 5 years ago

I have rebased onto your updated master and the tests all pass. I haven't added a test because I don't see any existing test for outlines and I'm not familiar with the framework you're using. Would you mind either accepting the change without a test, or writing one? In my consuming application, I tested each permutation of (outline={0, 6}) with (textOutline={0, 6, false}) and I think it looked right.

thw0rted commented 4 years ago

Totally forgot I didn't actually open a PR. Once this lands, what's the timeline look like for a release to NPM?

spatialillusions / milsymbol

Feature request: separate outline settings for text information only #225