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

EditMode RFC #212

Merged
merged 10 commits into from
Aug 23, 2019
Merged

EditMode RFC #212

merged 10 commits into from
Aug 23, 2019

Conversation

supersonicclay
Copy link
Collaborator

No description provided.

dev-docs/RFCs/v1.0/generic-edit-mode.md Outdated Show resolved Hide resolved
selectedIndexes: number[],
guides: ?TGuides,
onEdit: (data: TData, editContext: any) => void
onUpdateGuides: (guides: ?TGuides) => void,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add more details / descriptions about these parameters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done. Please review and let me know if there are questions about them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What does TGuides look like?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TGuides is intentionally abstract. For the BaseGeoJsonEditMode, it'll be a GeoJSON feature collection. But another type of edit mode (e.g. h3 hex editing) may use another data type for guides.

handleClick(event: ClickEvent): void {}
handlePointerMove(event: PointerMoveEvent): { cancelMapPan: boolean } {}
handleStartDragging(event: StartDraggingEvent): void {}
handleStopDragging(event: StopDraggingEvent): void {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. These methods are all public API? Could you differentiate between private and public ones?
  2. Is EditMode responsible of registering/deregistering events?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Good idea
  2. No. The handle... functions would be called by something that registers the event handlers (e.g. EditableGeoJsonLayer)

dev-docs/RFCs/v1.0/generic-edit-mode.md Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented May 4, 2019

Pull Request Test Coverage Report for Build 1374

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 36.276%

Totals Coverage Status
Change from base Build 1323: 0.0%
Covered Lines: 1032
Relevant Lines: 2366

💛 - Coveralls

dev-docs/RFCs/v1.0/generic-edit-mode.md Outdated Show resolved Hide resolved
dev-docs/RFCs/v1.0/generic-edit-mode.md Show resolved Hide resolved
dev-docs/RFCs/v1.0/generic-edit-mode.md Outdated Show resolved Hide resolved
supersonicclay added a commit that referenced this pull request May 8, 2019
This change includes a couple of breaking changes for the `translate` mode. Details are in changelog.

This is necessary in order to decouple the `ModeHandler` interface from deck.gl for the upcoming `EditMode` refactor (#212)

2. It is specific to GeoJSON. But there are desires to support editing other kinds of geometries (e.g. Hexagons using [H3](https://uber.github.io/h3/#/)).

## API
Copy link
Collaborator

@xintongxia xintongxia May 8, 2019

Choose a reason for hiding this comment

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

Could provide a simple example about how to use EditMode. I could also help with this since react-map-gl-draw is going to use this class.

Copy link
Collaborator Author

@supersonicclay supersonicclay Aug 14, 2019

Choose a reason for hiding this comment

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

I think this is achieved now for EditableGeoJsonLayer

* contains `EditableGeoJsonLayer`, a deck.gl `CompositeLayer`
* `@nebula.gl/geojson-modes`
* depends on `@nebula.gl/core` and [turf.js](http://turfjs.org/)
* contains all the modes for editing GeoJSON
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. List all the currently supported modes
  2. Maybe mention how to create a custom EditMode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@georgios-uber
Copy link
Contributor

Is this ready to merge?

@georgios-uber
Copy link
Contributor

@supersonicclay is this ready?

@supersonicclay
Copy link
Collaborator Author

I want to get a little farther with #222 before merging this one. Specifically I think we may want to document the BaseGeoJsonEditMode class also.

@georgios-uber
Copy link
Contributor

I want to get a little farther with #222 before merging this one. Specifically I think we may want to document the BaseGeoJsonEditMode class also.

Are we there yet?

@supersonicclay
Copy link
Collaborator Author

supersonicclay commented Jul 19, 2019

After talking through this in more detail with @xintongxia, we made some tweaks to the interface to make it more stateless and functional. Those tweaks are in #258, so will need to update this RFC after landing #258.

@supersonicclay supersonicclay force-pushed the clay/edit-mode-rfc branch 3 times, most recently from 77704f8 to d54326c Compare August 14, 2019 19:45
@supersonicclay supersonicclay merged commit d6f4f73 into master Aug 23, 2019
@supersonicclay supersonicclay deleted the clay/edit-mode-rfc branch August 23, 2019 18:58
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.

5 participants