-
Notifications
You must be signed in to change notification settings - Fork 409
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
Fix #2668 Increase precision of Query Panel Circle/BBox Details #2720
Conversation
this.props.onShowPanel(false); | ||
}; | ||
|
||
roundValue = (val, prec = 1000000) => Math.round(val * prec) / prec; | ||
getStep = (zoom = 1) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can translate this into a formula, in any case we shouldn't use ifs
const step = this.getStep(zoom); | ||
return name === 'radius' && step * 100000 || step; | ||
}; | ||
isWGS84 = () => (this.props.geometry || {}).projection === 'EPSG:4326' || !this.props.useMapProjection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use proj4 to know the uom of the projection, and generalize EPSG:4326 to uom === 'degrees'
@@ -77,7 +79,7 @@ class GeometryDetails extends React.Component { | |||
projection: this.props.geometry.projection | |||
}; | |||
|
|||
this.props.onChangeDrawingStatus("replace", undefined, "queryform", [geometry]); | |||
this.props.onChangeDrawingStatus({geometry: [geometry]}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is CHANGE_DRAWING_STATUS without a "command" (e.g. replace) supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe was better to change the name, but isn't directly calling the onChangeDrawingStatus action. That action is debounced and then called by enhancer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actions have a standard interface, you are changing it here
const propsStreamFactory = require('../../../misc/enhancers/propsStreamFactory'); | ||
|
||
|
||
const dataStreamFactory = $props => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done to debounce the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it also changes the interface to call onChangeDrawingStatus, which is not immediately clear, look at the other question a bove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see answer above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, if you put a "transparent" debounce in the middle, it should be transparent, if I remove it the same code will work, without debouncing. We want to enhance, not to change.
this.props.onShowPanel(false); | ||
}; | ||
|
||
getStep = (zoom = 1) => { | ||
if ( zoom >= 21 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we translate this into a formula?
const {compose, withHandlers} = require('recompose'); | ||
const {debounce} = require("lodash"); | ||
|
||
module.exports = compose(withHandlers((initProp) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we generalize this into a "debouncing" enhancer? I don't get the purpose of having single use enhancers in separate files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look on my PR for required changes.
I removed debouncing because it caused more issues tha benefits. Please add a test for your changes
const {debounce} = require("lodash"); | ||
const emptyFunc = () => {}; | ||
/** | ||
* This enhancer debounce an action or of a passed debounceTime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enhancer de-bounce a method passed as prop of the given time.
Description
If Query Panel Geometry details has proj 4326 the precision off numeric fields is increased to 6th decimal, the step is related to the current scale, and the update is debounced by 1 sec.
Issues
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
What is the current behavior? (You can also link to an open issue here)
All numeric fields has tow decimal precision, and the step is always one
What is the new behaviour?
If proj is 4326, coordinates fields has 6 decimal precision, the step is related to the scale and the update is debounced by one sec. The radius has 2 decimal precision.
Does this PR introduce a breaking change? (check one with "x", remove the other)
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: