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

reworking GridSystem to be based on latitude precision #32

Merged
merged 8 commits into from
Jun 17, 2017

Conversation

mridulnagpal
Copy link
Member

#18

@@ -13379,6 +13378,27 @@ BlurredLocation = function BlurredLocation(options) {
}
}

function gridWidthInPixels(degrees) {
var p1 = L.latLng(1,1);
Copy link
Member

Choose a reason for hiding this comment

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

Here, we can't just use 1,1 because that's a specific place on the map. We have to start with the current center -- so getLat() and getLon(), then add to those. It'll change based on where in the world you are, remember?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah , replaced the 1's with functions

@@ -13391,6 +13411,8 @@ BlurredLocation = function BlurredLocation(options) {
panMapWhenInputsChange: panMapWhenInputsChange,
panMap: panMap,
panMapByBrowserGeocode: panMapByBrowserGeocode,
gridWidthInPixels: gridWidthInPixels,
Copy link
Member

Choose a reason for hiding this comment

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

Also, these can be internal functions. They don't need an external API; that'll be getPrecision, later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually added them for testing, removing them.

@jywarren
Copy link
Member

@jywarren
Copy link
Member

For truncate, maybe we can just use a module: https://www.npmjs.com/search?q=truncate

maybe https://www.npmjs.com/package/jsdecimal ?

@@ -23,7 +23,7 @@ module.exports = function addGrid(map, onChangeLocation) {
include: L.Mixin.Events,

options: {
cellSize: 64,
cellSize: 40,
Copy link
Member

Choose a reason for hiding this comment

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

OK, so here you'd want to either use something like options.cellSize and have it settable via the constructor (like options.map, above), OR you could make addGrid have a method called addGrid.setCellSize(degreesWide, degreesTall)

@jywarren jywarren changed the title methods added reworking GridSystem to be based on latitude precision Jun 17, 2017
@mridulnagpal
Copy link
Member Author

@jywarren Please have a look made the changes

@@ -13315,7 +13318,7 @@ BlurredLocation = function BlurredLocation(options) {
return options.map.getSize();
}

addGrid = options.addGrid;
gridSystem = options.gridSystem;
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? I guess we can just refer to it by options.GridSystem, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can just use options.systemGrid removing it

@jywarren
Copy link
Member

OK, this looks good -- why don't we merge it and then do another one where you create a method called:

options.gridSystem.setCellSizeInDegrees(degrees);

And let's rename findPrecisionForMinimumGridWidth() to getMinimumGridWidth(pixels) and have it return { degrees: ____, precision: _____ } -- that way we can do:

var degrees = getMinimumGridWidth(100).degrees;
options.gridSystem.setCellSizeInDegrees(degrees);

Makes sense? setCellSizeInDegrees() will then adjust the geometry of the grid accordingly, and we can run it each time the zoom changes.

@jywarren
Copy link
Member

Once we do this, we'll need to modify GridSystem to be drawing the grid accordingly, and we can test it out.

@mridulnagpal
Copy link
Member Author

Cool, merging this one and working on the new methods

@mridulnagpal mridulnagpal merged commit 53c17c1 into publiclab:master Jun 17, 2017
@mridulnagpal mridulnagpal deleted the add-precision-options branch June 17, 2017 21:29
@mridulnagpal mridulnagpal restored the add-precision-options branch June 17, 2017 21:38
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.

2 participants