Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add a visible() and selectionAPI() functions to layers. #682

Merged
merged 3 commits into from
Apr 13, 2017

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Mar 28, 2017

Note that, much like feature level visibility, you might need to call layer.draw() after making the layer visible to ensure an update. For feature layers, visibility cascades down to individual features (which disables their interaction).

This resolves issue #674.

Note that, much like feature level visibility, you might need to call layer.draw() after making the layer visible to ensure an update.  For feature layers, visibility cascades down to individual features (which disables their interaction).
@codecov-io
Copy link

codecov-io commented Mar 28, 2017

Codecov Report

Merging #682 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #682      +/-   ##
==========================================
+ Coverage   95.09%   95.14%   +0.05%     
==========================================
  Files          83       83              
  Lines        8622     8678      +56     
==========================================
+ Hits         8199     8257      +58     
+ Misses        423      421       -2
Impacted Files Coverage Δ
src/polygonFeature.js 99.31% <ø> (ø) ⬆️
src/layer.js 84.87% <100%> (+1.54%) ⬆️
src/tileLayer.js 97.94% <100%> (+0.04%) ⬆️
src/featureLayer.js 100% <100%> (ø) ⬆️
src/feature.js 100% <100%> (ø) ⬆️
src/gl/quadFeature.js 99.51% <100%> (ø) ⬆️
src/pixelmapFeature.js 100% <100%> (ø) ⬆️
src/mapInteractor.js 96.12% <0%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac3de62...a3df5ce. Read the comment docs.

The active property can now be changed.
@manthey manthey changed the title Add a visible() function to layers. Add a visible() and selectionAPI() functions to layers. Mar 28, 2017
*
* @param {boolean|undefined} val: undefined to return the visibility, a
* boolean to change the visibility.
* @param {boolean} direct: if true, when getting the visibility, disregard
Copy link
Member

Choose a reason for hiding this comment

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

use of direct in two different ways is little confusing but I see a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

direct is only ever passed internally -- the user should never have to reference it. It is only so we know what the base visibility of a feature is regardless of the layer.

Copy link
Member

Choose a reason for hiding this comment

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

right but I could see a use-case (perhaps more complicated) where a user would like to know the visible state of a feature not w.r.t to its parent. But I agree that most likely they would just query the state w.r.t parent.

@@ -339,13 +339,14 @@ var pixelmapFeature = function (arg) {
m_quadFeature = m_this.layer().createFeature('quad', {
selectionAPI: false,
gcs: m_this.gcs(),
visible: m_this.visible()
visible: m_this.visible(undefined, true)
Copy link
Member

Choose a reason for hiding this comment

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

it would have been nice to just call it like m_this.visible(true). Should we switch the args placeholder?

Copy link
Member

@aashish24 aashish24 Apr 13, 2017

Choose a reason for hiding this comment

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

but that means that the user would have to provide answer to direct in their call, unless they are okay with passing undefined for direct in their code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The user just uses m_this.visible(true). This is just done, because we don't want to change the state of visibility based on the level.

Copy link
Member

Choose a reason for hiding this comment

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

Roger that, on the same page.

Copy link
Member

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

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

It looks reasonable to me, just has some minor questions..

@aashish24
Copy link
Member

I am going back and forth on direct. Typically in the past for state change / query I used bitwise operators that could provide bit more flexibility and clarification but that means that would have remember the magic does

For now like the names enough that I +2 it.

@aashish24
Copy link
Member

In summary:

  • User would mostly call visible(true)
  • When they really want to ensure they would call visible(true, true) [in this case I would have preferred force or override as arg names but direct works as well.

+2

@manthey
Copy link
Contributor Author

manthey commented Apr 13, 2017

The user really shouldn't ever call visible(true, true). Since the only time that visible(true) on a feature doesn't do something is if the layer is hidden, then when the layer's visibility changes, the layer uses the visible(..., true) to ensure that the interface hooks are enabled. If the layer's visibility isn't changing, adding the direct flag doesn't do anything useful when setting the feature visibility. When fetching the feature's visibility, it does allow telling whether the feature would be visible IF the layer is visible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants