Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Avoid setting marker opacity twice #5441

Merged
merged 7 commits into from
Feb 2, 2025

Conversation

erasta
Copy link
Contributor

@erasta erasta commented Feb 1, 2025

When creating a marker setOpacity was called twice, which results in calling _updateOpacity twice.
Also the opacity params are check twice for undefined.
This minor optimization solves the above.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

Copy link

codecov bot commented Feb 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.94%. Comparing base (cc9f170) to head (c3302c0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5441      +/-   ##
==========================================
- Coverage   91.95%   91.94%   -0.01%     
==========================================
  Files         282      282              
  Lines       39050    39049       -1     
  Branches     6863     6862       -1     
==========================================
- Hits        35907    35904       -3     
- Misses       3016     3017       +1     
- Partials      127      128       +1     

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

@HarelM
Copy link
Collaborator

HarelM commented Feb 1, 2025

Thanks for taking the time to fix this.
I've added a few minor comments.

@@ -857,16 +856,19 @@ export class Marker extends Evented {
* @param opacityWhenCovered - Sets the `opacityWhenCovered` property of the marker.
*/
setOpacity(opacity?: string, opacityWhenCovered?: string): this {
if (opacity === undefined && opacityWhenCovered === undefined) {
// Reset opacity when called without params or from constructor
if (this._opacity === undefined || (opacity === undefined && opacityWhenCovered === undefined)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this method can be simplified even further:

this._opacity = opacity !== undefined ? opacity : '1';
this._opacityWhenCovered = opacityWhenCovered!== undefined ? opacityWhenCovered : '0.2';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of calling marker.setOpacity(something);:

  • the expected result is that this._opacityWhenCovered will not be changed
  • in the suggested code it will reset it to the default '0.2'

same goes for calling marker.setOpacity(undefined, something);

I agree that a oneliner shortening is possible, but it will be somewhat less-understandable.

@HarelM HarelM merged commit fcbe9e5 into maplibre:main Feb 2, 2025
17 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants