Leaflet Map Panes #227

rjdestigter · 2016-09-23T16:46:33Z

Developed component implementing Leaflet's new 'panes' concept.

Component name: MapPane

Accepted props:

className
style
name (Not required, unique id wiill be created if not passed.)

Does Leaflet allow to override its existing panes?
No it does not seem like that. I've allowed passing a "blacklisted" name as prop.

Lifecycle:

constructor will create a unique id if no name is passed
componentDidMount will check if the pane exists or not and call map.createPane if not.
componentWillUnmount will remove the pane from the DOM and map
Added documentation and example.

…from context.

Conflicts: src/MapPane.js src/index.js

rjdestigter · 2016-09-28T14:34:22Z

Conflicts with next have been resolved.

PaulLeCam

Thanks for this PR, I think it's going in the right direction but it seems like many parts could be simplified.

PaulLeCam · 2016-09-29T06:27:29Z

docs/API.md

@@ -30,6 +30,7 @@ Check Leaflet documentation for the events associated to each component.
  - [BaseTileLayer](Components.md#basetilelayer)
  - [Path](Components.md#path)
 - [Map](Components.md#map)
+- [MapPanes](Components.md#map-pane)


Could you rename it to just Pane to match Leaflet API please?

PaulLeCam · 2016-09-29T06:27:51Z

docs/Components.md

@@ -11,6 +11,7 @@ You can directly access the Leaflet element created by a component using `this.l
  - [MapLayer](#maplayer)
  - [Path](#path)
 - [Map](#map)
+- [MapPane](#map-pane)


MapPane -> Pane

PaulLeCam · 2016-09-29T06:32:58Z

src/types/pane.js

+/* @flow */
+import { PropTypes } from 'react'
+
+export default PropTypes.string


No need to create an extra PropType if it is as simple as a string I think.

PaulLeCam · 2016-09-29T06:33:19Z

src/index.js

@@ -24,3 +24,4 @@ export TileLayer from './TileLayer'
 export Tooltip from './Tooltip'
 export WMSTileLayer from './WMSTileLayer'
 export ZoomControl from './ZoomControl'
+export MapPane from './MapPane'


MapPane -> Pane

PaulLeCam · 2016-09-29T06:36:44Z

src/MapPane.js

+      _isMounted: false,
+    }
+
+    this._name = name || `pane-${uniqueId()}`


No need for this._name and this.state._isMounted, we could just have this.state.name to check if the children should be rendered.

PaulLeCam · 2016-09-29T07:05:17Z

src/MapPane.js

+    const map = this.context.map || this.props.map
+
+    if (this._name && map && map._panes) {
+      map._panes = omit(map._panes, this._name)


Seems a bit of a hack, is there any other way to cleanup the pane from Leaflet using more public APIs?

There isn't at the moment as far I can find. Browsing Leaflet's source code I could only find:

createPane, getPane, and getPanes

Calling L.DomUtil.remove(pane) only removes it from the DOM but not the references leaflet keeps in map._panes and map._paneRenderers

I had to dig for _paneRenderers. It was keeping a reference to the detached pane node causing leaflet to render layers inside it if the pane was added back again.

I find this 'hacky' as well but I couldn't find a cleaner method or api for it at the moment.

OK thanks for the explanation, hopefully Leaflet won't remove these anytime soon!

PaulLeCam · 2016-09-29T07:06:47Z

src/MapPane.js

+    name: PropTypes.string,
+    children: childrenType,
+    map: mapType,
+    zIndex: PropTypes.number,


zIndex should be part of the style, unless there is a particular reason to treat it differently?

Syntactic sugar. Since the main purpose of leaflet panes is to have more control over of zIndexes I included it as a prop. Let me know if you want this dropped.

Yes I think it's better to just rely on style, I prefer to make the components as "dumb" as possible with no specific logic and let the applications implement custom behavior on top of them.

PaulLeCam · 2016-09-29T07:08:13Z

src/MapPane.js

+    this.setStyle(props)
+  }
+
+  setStyle ({ style, zIndex } = this.props) {


Shouldn't need that, React can render the styles itself.

Yes, but React in this case is not in charge of the DOM node right?

Yes, sorry I missed that from Leaflet's createPane() implementation.

PaulLeCam · 2016-09-29T07:08:33Z

src/MapPane.js

+    }
+  }
+
+  componentWillReceiveProps (props) {


Shouldn't be needed.

Since React is unaware of the actual DOM node, I have to apply incoming style changes to the pane node.

I see, yes I missed the fact Leaflet is creating a new DOM node rather than using the one provided. It's too bad it doesn't allow it as we could have completely leveraged React for this, but in that case I think your implementation is needed as such.

PaulLeCam · 2016-09-29T07:09:19Z

src/MapComponent.js

@@ -72,4 +72,17 @@ export default class MapComponent extends Component<any, any, any> {
    const el = this.leafletElement
    if (el) el.fire(type, data)
  }
+
+  getInstanceOptions (props = {}) {


Could be simply named getOptions()

PaulLeCam · 2016-09-30T06:52:07Z

@rjdestigter thanks for the changes and explanations.
My only major concern remaining is with allowing to override an existing pane, whether it's a default Leaflet one or another one already created, but I'm happy to merge this PR as-is and work on it myself if you prefer?

rjdestigter · 2016-09-30T14:30:26Z

I suggest we still allow using default leaflet panes but don't set their style in that case. Pane doesn't actually really override existing panes, it just receives them from map.getPane and thus won't create them since it already exists.

Todo's:

Remove zIndex prop
Disallow usage of existing panes unless they are default leaflet panes
Don't apply style to default panes. (Disallow style props for default panes)

rjdestigter · 2016-09-30T15:35:54Z

I also realised className was being completely ignored so I added that in as well. Proper behaviour was added to deal with this.props.name changes.

Also, yes, let's disallow using default names. I thought I needed that feature, but if I want other components such as Popup or any component for that matter to be added to a default pane even if it is nested inside a Pane component, I can just use prop.pane for them.

<Pane name='paris'>
  <GeoJSON data={...}>
    <Popup pane={'popupPane'}

I will disallow default pane names.

rjdestigter · 2016-09-30T15:56:07Z

Ok, done, using default pane names or that of existing panes will throw an error.

rjdestigter · 2016-09-30T15:58:08Z

but I'm happy to merge this PR as-is and work on it myself if you prefer?

I'm good with that if you approve of today's changes. The only thing that I'd like to revisit myself is getChildren, but you are welcome to that as well.

I'm not sure if React.cloneElement is really necessary. map is available through context and if apps using Pane need it to pass down props, they can always extend Pane and override getChildren

rjdestigter · 2016-09-30T16:01:53Z

Also, thanks by the way for letting me collaborate on this project!

rjdestigter · 2016-09-30T17:01:30Z

I acutally went ahead and removed the usage of cloneElement. I tested it using nested Panes and it's working fine.

Also when map.createPane is called I store the used name in this.paneName. This was necessary because in some cases componentWillUnmount is called before this.setState({name}) is persisted to local state causing Pane not to remove the pane.

PaulLeCam · 2016-10-04T07:03:00Z

Thanks for this PR!
I'll need to play a bit with it to make sure I catch possible edge cases before releasing, but hopefully it should make it to the next RC.

John de Stigter added 10 commits September 23, 2016 10:30

MapPane implements Leaflet panes.

8511471

Added MapPane to index.js

c1facae

Helper function 'getInstanceOptions' to pass options + possible pane …

8b8d071

…from context.

Implemented possible pane context into appropriate items.

8984a61

Added MapPane to docs and examples.

435b94b

Added to docs/API

959f000

Typo fix

ca63316

Typo fix

7e4d896

Initial commit for MapPane component.

6085d0a

Merge branch 'next' into LeafletMapPanes

35bb20d

Conflicts: src/MapPane.js src/index.js

John de Stigter added 2 commits September 28, 2016 10:16

Allow overriding pane name through props.

f1474ca

Allow nested panes.

3f9c9d1

PaulLeCam requested changes Sep 29, 2016

View reviewed changes

John de Stigter added 11 commits September 29, 2016 11:10

Renamed 'MapPane' to 'Pane'

83bf50a

Renamed 'getInstanceOptions' to 'getOptions'

cae42e2

Renamed MapPane.js to Pane.js

37597d6

Removed paneType

adfdd14

Renamed MapPane to Pane in documentation and examples.

a550af9

Removed _isMounted, Moved this._name to this.state.name

b94a065

Bugfix missing 'PropTypes'

71ed686

componentWillMount moved to componentDidMount

406d518

Removed paneType export from types/index

3873fcb

Added Pane export in index to alphabetical order.

7d37ea7

Updated docs and example.

a6b6c6f

PaulLeCam approved these changes Sep 30, 2016

View reviewed changes

PaulLeCam mentioned this pull request Sep 30, 2016

Panes #219

Closed

Comments, className, removed zIndex.

a3a8a1c

John de Stigter added 2 commits September 30, 2016 09:17

Updated pane docs and example.

f70403a

Removed deprecated lines in createPane.

8329cdf

John de Stigter added 2 commits September 30, 2016 09:49

Disallow default pane names.

9a2e259

Updated docs

13e7d0b

Removed 'cloneElement' for children, Bugfix removing pane.

bbb70c3

PaulLeCam merged commit c2be3d1 into PaulLeCam:next Oct 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leaflet Map Panes #227

Leaflet Map Panes #227

rjdestigter commented Sep 23, 2016 •

edited

Loading

rjdestigter commented Sep 28, 2016

PaulLeCam left a comment

PaulLeCam Sep 29, 2016

rjdestigter Sep 29, 2016 •

edited

Loading

PaulLeCam Sep 29, 2016

rjdestigter Sep 29, 2016 •

edited

Loading

PaulLeCam Sep 29, 2016

rjdestigter Sep 29, 2016

PaulLeCam Sep 29, 2016

rjdestigter Sep 29, 2016

PaulLeCam Sep 29, 2016

rjdestigter Sep 29, 2016

PaulLeCam Sep 29, 2016

rjdestigter Sep 29, 2016 •

edited

Loading

PaulLeCam Sep 30, 2016

PaulLeCam Sep 29, 2016

rjdestigter Sep 29, 2016

PaulLeCam Sep 30, 2016

PaulLeCam Sep 29, 2016

rjdestigter Sep 29, 2016

PaulLeCam Sep 30, 2016

PaulLeCam Sep 29, 2016

rjdestigter Sep 29, 2016

PaulLeCam Sep 30, 2016

PaulLeCam Sep 29, 2016

rjdestigter Sep 29, 2016

PaulLeCam commented Sep 30, 2016

rjdestigter commented Sep 30, 2016 •

edited

Loading

rjdestigter commented Sep 30, 2016

rjdestigter commented Sep 30, 2016

rjdestigter commented Sep 30, 2016 •

edited

Loading

rjdestigter commented Sep 30, 2016

rjdestigter commented Sep 30, 2016

PaulLeCam commented Oct 4, 2016

Leaflet Map Panes #227

Leaflet Map Panes #227

Conversation

rjdestigter commented Sep 23, 2016 • edited Loading

rjdestigter commented Sep 28, 2016

PaulLeCam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjdestigter Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjdestigter Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjdestigter Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulLeCam commented Sep 30, 2016

rjdestigter commented Sep 30, 2016 • edited Loading

rjdestigter commented Sep 30, 2016

rjdestigter commented Sep 30, 2016

rjdestigter commented Sep 30, 2016 • edited Loading

rjdestigter commented Sep 30, 2016

rjdestigter commented Sep 30, 2016

PaulLeCam commented Oct 4, 2016

rjdestigter commented Sep 23, 2016 •

edited

Loading

rjdestigter Sep 29, 2016 •

edited

Loading

rjdestigter Sep 29, 2016 •

edited

Loading

rjdestigter Sep 29, 2016 •

edited

Loading

rjdestigter commented Sep 30, 2016 •

edited

Loading

rjdestigter commented Sep 30, 2016 •

edited

Loading