673931956 - Update FreeDrawLayer to accommodate multiple shapes #372
Conversation
grenti
force-pushed
the
dh/673931956/allow-multiple-shapes-react-ui
branch
from
9678884 to
d374ca4
Compare
|
Will do - we realized we didn’t use it earlier and so I was going to update the pr with tests and git after I remove WIP. |
|
|
||
| if (polygonCoords.length) this.finishDraw(polygonCoords) | ||
|
|
||
| this.endDraw() |
There was a problem hiding this comment.
I think you need to add tests to make sure these two get fired here.
| const { enabled, shapes } = this.state | ||
|
|
||
| if (!enabled && (Object.keys(shapes).length > 0)) { | ||
| this.setState({ shapes: existingShapes }) |
There was a problem hiding this comment.
Can we combine this setState with the one below so we don't have a double render?
grenti
force-pushed
the
dh/673931956/allow-multiple-shapes-react-ui
branch
from
8a9128c to
4b296e4
Compare
| handleClick() { | ||
| this.setState({ enabled: !this.state.enabled }) | ||
| const existingShapes = this.props.shapes | ||
| const { enabled, shapes: _ } = this.state |
There was a problem hiding this comment.
_ usually denotes you aren't using that prop. Cant' we just rename it to something else? I'd prefer the code below to be readable.
| const existingShapes = this.props.shapes | ||
| const { enabled, shapes: _ } = this.state | ||
|
|
||
| const shapes = !enabled && Object.keys(_).length > 0 && _ !== existingShapes |
There was a problem hiding this comment.
Object.keys(_).length > 0 should just be Object.keys(_).length
There was a problem hiding this comment.
I also don't think we should really be using both props and state to write conditionals. What happens if props.shapes changes is that going to be an issue? There isn't just 1 single source of truth here.
There was a problem hiding this comment.
I think I'm going to take out the state shape logic - I think it was intended to store the shapes and now since we are trying to reconcile both it's getting out of hand. I'll just reset the state on click back to the props passed in.
| this.setState({ | ||
| shapes: shape && { 0: shape }, | ||
| enabled: false, | ||
| shapes: shape && { ...shapes, [shapeId]: shape }, |
There was a problem hiding this comment.
I thought you were removing this?
There was a problem hiding this comment.
I removed the logic from handleClick that updated the shapes in state. The way the component is written we have to use the state to keep all the shapes that have been drawn in the Multi-shape scenario. We can refactor this component to handle multi-shapes but since we're trying to get this released on Thursday I was trying to be as conservative as possible.
tadjohnston
left a comment
tadjohnston
left a comment
There was a problem hiding this comment.
Oops, still need to squash and use one git cz style commit 😄
|
Ok I'll squash - I was still making changes based on our conversation but we do need to get this merged so we can publish and use for ag.js changes going in tomorrow. |
…ed before drawing multiple shap affects: @rentpath/react-ui-core * updated FreeDrawLayer to accomodate multiple shapes * took out logic for Google Map Polyline and Polygon api from disableDraw * put it in endDraw * called endDraw in disableDraw, handleMouseUp * Updated FreeDrawExample to show multi-shape use case
grenti
force-pushed
the
dh/673931956/allow-multiple-shapes-react-ui
branch
from
0763c40 to
9ea5ade
Compare
Card
affects: @rentpath/react-ui-core