Add Marker's "opacity" option and "setOpacity" method

[sbachinin]

Implements #3458

A small caveat here.
This solution means that all users have to provide an array of 2 strings. (2nd string is the reduced opacity when marker is behind the terrain).
But in most cases (without 3d terrain) this second string will be useless and just an incovenience.
Perhaps it makes sense to split this single option and method to:
'opacity/setOpacity' and 'reducedOpacity/setReducedOpacity'

[HarelM]

Why not split this into two fields?
opacity and opacityWhenHidden
Or choose better words.
The setOpacity method can receive the second parameters as optional that defaults to 1 or doesn't change the internal value if not provided.

[neodescis]

Also, these values should really be numbers instead of strings.

[HarelM]

In theory, opacity can be 50% instead of 0.5, but I agree, a number would be better from an API perspective.

[sbachinin]

Now setOpacity accepts 2 numbers.
If any of them is undefined, a respective state value (_opacity or _opacityWhenCovered) remains unchanged.
If called without arguments, does nothing.

[codecov-commenter]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (0e27f8b) 87.42% compared to head (efe0104) 87.01%.

Files	Patch %	Lines
src/ui/marker.ts	96.15%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3620      +/-   ##
==========================================
- Coverage   87.42%   87.01%   -0.41%     
==========================================
  Files         240      240              
  Lines       32189    32212      +23     
  Branches     2067     2253     +186     
==========================================
- Hits        28140    28030     -110     
- Misses       3139     3242     +103     
- Partials      910      940      +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[HarelM]

Can you also merge from main branch?
I want to see that the new code coverage is working as expected.
There should not be a message saying that 3 lines are missing coverage as you have added tests to cover the code you added.
The missing lines are ending brackets, main is now updated with this fix, but I want to see it actually fixes this issue.

[sbachinin]

Done.

Merged from main. After that npx jest stopped working for me locally. (I use it because the doc suggests using it to run individual tests).
The only output in the console for me is "Determining test suites to run...".
(Previously I saw results of passed and failed tests).
Managed to run tests using npm test-unit, it seems ok.

[HarelM]

Yes, it's due to the new reported I added.
You should be able to use npx jest --reporter=default ...
I should probably update the docs somewhere...

[HarelM]

Can you update the build test with the extra size?
Although this is strange that this change adds a significant bundle size...

[HarelM]

@sbachinin first and foremost thanks for all the work and the review changes, I know I can by annoying sometimes...
I see in the code coverage report that one of the lines is partially covered.
According to the tests you added I think this is not a correct report - i.e. I think the coverage report tool is not functioning well with the ? statement.
Can you make sure both options are covered before I submit a bug report (using logging, or however you think is the fastest option)?

CC: @cenfun

[sbachinin]

Seems to be a problem with my test. In all test cases marker "is invisible" though I expected it to be visible in one case.
So yes, coverage report is right.
I'll fix the test.

[HarelM]

Ok, great! This means that the report is working as expected and found a place that is not covered! Yay!

[cenfun]

@HarelM I'm glad to know this.

[sbachinin]

In order to make this line fully covered, I need a very delicate scenario when the marker's base is invisible but its center is visible. I'm not sure if it makes sense to test this.

[HarelM]

I'll merge this as this feature wasn't introduced by you.

@SnailBones can you please open a PR with a test that covers this scenario?
I believe this is the only way to make sure the behavior is kept in the future.

[SnailBones]

@SnailBones can you please open a PR with a test that covers this scenario? I believe this is the only way to make sure the behavior is kept in the future.

@HarelM I don't think this is possible with a unit test, since AFAIK we don't have a way to set real terrain in unit tests. Maybe a browser test could work?

[HarelM]

I believe you can mock what you need, can't you?

[sbachinin]

If you don't mind I have an idea how to cover the case of semi-visible marker.
It feels really hacky to me though.
sbachinin@f49d0a2

[HarelM]

Looks fine to me, you mock what you need to cover this use case.