search

Vizzuality / layer-manager

A library to get a layer depending on provider and layer spec
https://layer-manager-docs.vercel.app
MIT License
18 stars 12 forks source link

Feat: more tests #74

Closed sorodrigo closed 5 years ago

sorodrigo commented 5 years ago

Please review and merge after #72 and #73

This PR adds more tests. I found some weird use cases that commented on the tests. I think I found and fixed one bug at utils/vector-styles-layers.js.

I think I also found another bug in the layer manager core, for some reason we're using plugin.setOpacity to toggle the visibility withing the update method. I think this is wrong, because it's not compatible with Leaflet and IMO is responsibility of the plugin not the core. Also it seems weird to have it done in that way in update and not in setVisibility. Finally I think, this also loses the original opacity of the layer once you toggle it to hidden and then back to visible.

P.S. I don't have access to the settings repo, therefore can't set up the CI on travis or whatever. Would be good to set that up with a coverage bot as well..

edbrett commented 5 years ago

Can you say what the bug you found in the vector styles parser is? From the code changes I cannot see a different in logic, only in syntax.

mbarrenechea commented 5 years ago

I think he means this https://github.com/Vizzuality/layer-manager/pull/74/files#diff-c7a2f45e404ef034e590237f291ae49cR31

He is also checking for null values

sorodrigo commented 5 years ago

Exactly, because null gets typecasted as 0, cuz javascript. So, null * 0.4 === 0