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 new mode for drawing a circle #4

Closed
wants to merge 7 commits into from
Closed

Add a new mode for drawing a circle #4

wants to merge 7 commits into from

Conversation

haixiangyan
Copy link

Summary

I just make a new circle mode (ScaleCircleMode) for drawing a circle by moving cursor. Here's the gif demo:

Demo

const Constants = require('@mapbox/mapbox-gl-draw/src/constants');
const circle = require('@turf/circle');

describe('CircleMode tests', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

Rename the mode to ScaleCircleMode

ScaleCircleMode.onSetup = function () {
const polygon = this.newFeature({
type: Constants.geojsonTypes.FEATURE,
properties: {
Copy link
Owner

Choose a reason for hiding this comment

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

add the radius too, to the properties.


// Compute circle attributes
const radius = turf.distance(startPoint, currentPoint) / 2;
const midPoint = turf.midpoint(startPoint, currentPoint);
Copy link
Owner

Choose a reason for hiding this comment

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

usability issue: better to have midpoint as the center of the circle and radius as just the distance between the current point and the start point


// Reset circle
state.polygon.incomingCoords(circleFeature.geometry.coordinates);
state.polygon.properties.center = midPoint.geometry.coordinates;
Copy link
Owner

Choose a reason for hiding this comment

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

compute the radius in kms and add it to the properties


ScaleCircleMode.onClick = function (state, e) {
// First click
if (state.currentVertexPosition === 0) {
Copy link
Owner

@iamanvesh iamanvesh Jul 1, 2019

Choose a reason for hiding this comment

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

usability: better to add this in onMouseDown

Suggested change
if (state.currentVertexPosition === 0) {
ScaleCircleMode.onMouseDown = ScaleCircleMode.onTouchStart = function (state, e) {
if (state.currentVertexPosition === 0) {

}
// Second click
else {
return this.changeMode(Constants.modes.SIMPLE_SELECT, {featureIds: [state.polygon.id]});
Copy link
Owner

@iamanvesh iamanvesh Jul 1, 2019

Choose a reason for hiding this comment

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

move this to onMouseUp

Suggested change
return this.changeMode(Constants.modes.SIMPLE_SELECT, {featureIds: [state.polygon.id]});
ScaleCircleMode.onMouseUp = ScaleCircleMode.onTouchEnd = function (state, e) {
return this.changeMode(Constants.modes.SIMPLE_SELECT, {featureIds: [state.polygon.id]});
}

};

ScaleCircleMode.onMouseMove = function (state, e) {
if (state.currentVertexPosition === 0) {
Copy link
Owner

@iamanvesh iamanvesh Jul 1, 2019

Choose a reason for hiding this comment

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

move all this processing to dragMove and touchmove

Suggested change
if (state.currentVertexPosition === 0) {
ScaleCircleMode.dragMove = ScaleCircleMode.onTouchMove = function (state, e) {
if (state.currentVertexPosition === 0) {

package.json Outdated
@@ -22,7 +22,8 @@
"@turf/circle": "^6.0.1",
"@turf/distance": "^6.0.1",
"@turf/helpers": "^6.1.4",
"@turf/length": "^6.0.2"
"@turf/length": "^6.0.2",
"@turf/turf": "^5.1.6"
Copy link
Owner

Choose a reason for hiding this comment

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

import the modules that are necessary, turf is a huge library which increases the build size significantly. revert the package-lock.json file and the package.json before adding the dependencies.

Copy link
Owner

@iamanvesh iamanvesh left a comment

Choose a reason for hiding this comment

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

please update the PR addressing the usability and code related issues, if you can.

@haixiangyan haixiangyan closed this Jul 2, 2019
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.

None yet

2 participants