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

Create a consistent convention for getters, setters, and constructor options #4029

Open
lucaswoj opened this issue Jan 20, 2017 · 11 comments
Open
Labels
api 📝 breaking change ⚠️ Requires a backwards-incompatible change to the API

Comments

@lucaswoj
Copy link
Contributor

Many attributes of the map have getters, setters, and constructor options (for example, minZoom, getMinZoom, setMinZoom 👍 )

Some have only a subset of the three, even though it would be practical and useful to have all three. (for example, maxBounds and setMaxBounds exist but getMaxBounds does not) (for example, renderWorldCopies exists as a constructor option but we do not provide setRenderWorldCopies or getRenderWorldCopies).

We should do a review of all getters, setters, and constructor options to ensure that

  • for every constructor option, there exists a getter
  • for every constructor option that could be changed at runtime, there exists a setter
  • for every getter that could be set in the constructor, there exists a constructor option
  • for every setter, there exists a getter

We may want to create some infrastructure to automatically plumb together constructor options and getters.

@anandthakker
Copy link
Contributor

We may want to create some infrastructure to automatically plumb together constructor options and getters

@lucaswoj might be worth considering doing this auto-plumbing in conjunction with #2741

@mollymerp
Copy link
Contributor

mollymerp commented Jan 23, 2017

Opened #4039 before I saw this issue!

@lucaswoj
Copy link
Contributor Author

We may want to consider using ES6 getters / setters in leu of getFoo / setFoo methods.

@lamuertepeluda
Copy link

I'd love to have getters/setters for each properties. While this gets implemented, I found a small workaround for setting interaction on/off (currently only possible per constructor settings) and getMaxBounds (there is a setter, but strangely no getter for this).

In this snippet, map is an instance of mapboxgl.Map

// A collection of handler names
const mapInteractionHandlers = [
    'scrollZoom', 'dragPan', 'touchZoomRotate', 'dragRotate', 'keyboard', 'boxZoom', 'doubleClickZoom'
];

// Returns true if at least one of the above handlers is enabled
function isInteractionEnabled(map) {
    return mapInteractionHandlers.reduce((isEnabled, nextHandler) => {
        return map[nextHandler].isEnabled() && isEnabled;
    }, true);
}

// Enable map interaction (all handlers)
function enableInteraction(map) {
    for (let handler of mapInteractionHandlers) {
        map[handler].enable();
    }
}

// Disable map interaction (all handlers)
function disableInteraction(map) {
    for (let handler of mapInteractionHandlers) {
        map[handler].disable();
    }
}

// Returns true if the maxBounds have been set using the corresponding setter
function hasMaxBounds(map) {
    return (!Array.isArray(map.transform.latRange) || map.transform.latRange.length === 0) &&
        (!Array.isArray(map.transform.lngRange) || map.transform.lngRange.length === 0);
}

// Returns maxBounds from map.transform as Array<Array> if they are set
// or null
function getMaxBounds(map) {
    if (!hasMaxBounds(map)) {
        return null;
    }
    else {
        let transform = map.transform;
        let { latRange, lngRange } = transform;
        let ne = [lngRange[1], latRange[1]];
        let sw = [lngRange[0], latRange[0]];
        return [ne, sw];
    }
}

@andrewharvey
Copy link
Collaborator

@lamuertepeluda are you interested in submitting a PR based on that? It looks like you should be able to simply place this code within GL JS just adding JS doc and tests.

andrewharvey added a commit that referenced this issue Jun 26, 2017
Credit to @lamuertepeluda which provided some of the basis to this
commit at #4029 (comment)
@andrewharvey
Copy link
Collaborator

I've attempted to implement a getMapBounds at #4890.

/cc @lamuertepeluda

@lamuertepeluda
Copy link

@andrewharvey thanks, I'm sorry I couldn't do it but I'm happy if somebody gets to integrate this stuff... I think it's useful to have all setters and getters coherently for each parameter.

I also hope the E6/7 setters/getters convention will be used in the next releases.

I'd also like a function for atomically set/get all or some map state parameters for serialization/deseralization and other purposes, e.g. something as

map.setState({
  zoom,
  center,
  pitch,
  bearing,
  boundingBox,
 ...
});

let mapState = map.getState();  // mapState = {zoom, center, pitch, bearing, boundingBox, ...}

This would be helpful in combination with state management libraries such as redux, or simply for storing and reading the map state in the local storage or remotely...

@rares-lupascu
Copy link

so are the methods available?
enableInteraction
disableInteraction

@waissbluth
Copy link

also curious about this, specifically interested in pitchWithRotate

@allison-strandberg
Copy link
Contributor

Seconding interest in a pitchWithRotate setter, as a previous approach to setting this value no longer works in v1.10.0. #9666

@andrewharvey
Copy link
Collaborator

get/set cooperativeGestures would be nice. For example to disable it when the map takes up the full window (so no page scroll, but not fullscreen at the browser level).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api 📝 breaking change ⚠️ Requires a backwards-incompatible change to the API
Projects
None yet
Development

No branches or pull requests

9 participants