move SplitScreen & SyncableMap into ol-kit #362
Conversation
stazrad
requested review from
DAndrews99,
PatchesMaps,
akuma1,
chrisreams91,
drodenberg,
gabe647 and
introvertedspud
as code owners
|
|
||
| /** | ||
| * @component | ||
| * @category vmc |
app/demos/multi/App.js
Outdated
| numberOfColumns={4}> | ||
| <Map id={key} onMapInit={this.onMapInit} isMultiMap> | ||
| <Map map={map} onMapInit={this.onMapInit} isMultiMap> | ||
| {/* <SplitScreen /> */} |
There was a problem hiding this comment.
Should this still be here commented out? Is having a SplitScreen on another demo better?
app/index.html
Outdated
| <path d="M115.0,115.0 C114.9,115.1 118.7,116.5 119.8,115.4 L133.7,101.6 C136.9,99.2 139.9,98.4 142.2,98.6 C133.8,88.0 127.5,74.4 143.8,58.0 C148.5,53.4 154.0,51.2 159.7,51.0 C160.3,49.4 163.2,43.6 171.4,40.1 C171.4,40.1 176.1,42.5 178.8,56.2 C183.1,58.6 187.2,61.8 190.9,65.4 C194.5,69.0 197.7,73.2 200.1,77.6 C213.8,80.2 216.3,84.9 216.3,84.9 C212.7,93.1 206.9,96.0 205.4,96.6 C205.1,102.4 203.0,107.8 198.3,112.5 C181.9,128.9 168.3,122.5 157.7,114.1 C157.9,116.9 156.7,120.9 152.7,124.9 L141.0,136.5 C139.8,137.7 141.6,141.9 141.8,141.8 Z" fill="currentColor" class="octo-body"></path> | ||
| </svg> | ||
| </a> | ||
| <div id="map0"></div> |
There was a problem hiding this comment.
Might be nice to call out that these id values are expected to match those in App.js line 27...
src/Map/Map.js
Outdated
| // if no map was passed, create the map | ||
| this.map = !this.passedMap ? createMap({ target: this.target }) : passedMap | ||
|
|
||
| console.log('<Map> did mount', this.passedMap, !this.passedMap.getTargetElement()) |
| onClick={toggleSyncMap.bind(this, index)}> | ||
| {disabled | ||
| ? <LockIcon className={'zmdi zmdi-lock'}/> | ||
| : <div>{translations['_ol_kit.MapDisplayElement.map']} {mapNumber}</div> |
There was a problem hiding this comment.
I know this is way outside the scope of this change, but is there a story/practice/guidance for doing the localized number rendering here? Does falling back the browser's localized number formatting make sense in the context of translations or is there some better way of ensuring that the selected translation's language matched the number formatting of the mapNumber?
|
|
||
| MapDisplayElement.defaultProps = { | ||
| translations: { | ||
| '_ol_kit.MapDisplayElement.map': 'Map' |
There was a problem hiding this comment.
... e.g. above... is there support for positional parameters and such in these translations?
| import DragBarIcon from './svgIcons/DragBarIcon' | ||
|
|
||
| const Slider = ({ initialPosition, onDrag, innerRef }) => { | ||
| const limit = window.innerWidth * 0.2 |
There was a problem hiding this comment.
Should we be using window here or some parent container (so we are able to contain the map set inside another container)?
There was a problem hiding this comment.
valid- look into this
| const { right } = maps[0]?.getTargetElement()?.getBoundingClientRect() | ||
|
|
||
| if (right === window.innerWidth && !startPosition) { | ||
| this.setState({ startPosition: window.innerWidth / 2 }) |
There was a problem hiding this comment.
Could/should the initial split be settable? Right now it's forced to 50/50...
src/SplitScreen/SplitScreen.js
Outdated
| syncedMaps.map(map => { | ||
| const thisView = map.getView() | ||
|
|
||
| if (thisView !== view && !thisView.getAnimating()) { |
There was a problem hiding this comment.
Can thisView be null/undefined? Do we need to early exit?
|
|
||
| addMap = () => { | ||
| const { onMapAdded, maps, visibleMapCount } = this.props | ||
| const nextMapIdx = visibleMapCount |
There was a problem hiding this comment.
Do we need to handle the case where visibleMapCount is zero?
There was a problem hiding this comment.
we dont support a zero map workflow
| } | ||
| <MapDisplayContainer disabled={disabled}> | ||
| {visibleState.map((visible, i) => { | ||
| const grow = i === 0 && visibleMapCount === 3 |
There was a problem hiding this comment.
What's the meaning of 3 here? It this trying to force a new row every two or something? Looks like we're trying to put a slider between every other map... so I'm wondering if this should really be some visibleMapCount % 4 === 3 math?
| } | ||
| })} | ||
| </MapDisplayContainer> | ||
| {startPosition && visibleMapCount === 2 && |
There was a problem hiding this comment.
What about when visibleMapCount gets beyond 3 (e.g. 4 or other even numbers thereafter)? Should the be visibleMapCount % 2 === 0 math?
src/SplitScreen/utils.js
Outdated
| } | ||
| } | ||
|
|
||
| export function coordDiff (coord1, coord2, view) { |
There was a problem hiding this comment.
This might be better refactored to take the projection directly instead of a view that we only use to call getProjection from... more general-purpose that way and have less property-envy
src/SplitScreen/utils.js
Outdated
| } | ||
|
|
||
| export function targetDestination (startCoord, distance, bearing, view) { | ||
| const projection = view.getProjection() |
There was a problem hiding this comment.
IDisposable
left a comment
IDisposable
left a comment
There was a problem hiding this comment.
Looks pretty great, sorry for my intrusive queries... just learning more here.
|
@IDisposable i pushed this up with a |
src/MultiMapManager/SafeParent.js
Outdated
| const parentContextKey = keys.find(key => current.closest(`#${key}`) || current.closest(`#${key} ~ *`)) // search the dom, starting at the placeholder ref created in the initial render and moving up; searching first for the map div itself and then siblings of the map div to handle how the map component currently handles children. | ||
|
|
||
| if (!parentContextKey) ugh.error(`Could not find parent <Map> for component: "${Component.name}" during context lookup (tip: make sure portals render as children of their map.getTarget() parent)`) // eslint-disable-line max-len | ||
| if (!parentContextKey && Component.name !== 'Map') ugh.error(`Could not find parent <Map> for component: "${Component.name}" during context lookup (tip: make sure portals render as children of their map.getTarget() parent)`) // eslint-disable-line max-len |
There was a problem hiding this comment.
fix this to check explicitProps.isMultiMap since component name may change when bundled
src/MultiMapManager/FlexMap.js
Outdated
| const visibleMapCount = visibleState.filter(_=>_).length | ||
| // TODO also check for index against total to see if its on a row with 1 or 2 collumns | ||
| const columns = numberOfColumns || | ||
| (visibleMapCount === 1 || visibleMapCount === 3) && (index !== 0) ? 1 : 2 |
There was a problem hiding this comment.
(visibleMapCount & 1 === 1) && (index !== 0) ? 1 : 2 would work for ANY odd number of visible maps
| const { contextProps, translations } = this.props | ||
| const { maps } = this.state | ||
| const map = maps[0] | ||
| // console.log('getContextValue', { |
There was a problem hiding this comment.
commented code needs to be removed.
src/Map/Map.js
Outdated
| <StyledMap | ||
| id={this.target} | ||
| fullScreen={fullScreen} | ||
| show={true} |
There was a problem hiding this comment.
this can be dropped
| latitude | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
this should not be deleted
| ...config, | ||
| synced, | ||
| visible, | ||
| view: map.getView() |
There was a problem hiding this comment.
is this causing white map bug?
| // this is the first map, set the view in state | ||
| this.syncedView = map.getView() | ||
| } else { | ||
| map.setView(this.syncedView) |
There was a problem hiding this comment.
this may also be the cause of white map?
| ? <LockIcon className={'zmdi zmdi-lock'}/> | ||
| : <div>{translations['_ol_kit.MapDisplayElement.map']} {mapNumber}</div> | ||
| } | ||
| </PrimaryButton> |
There was a problem hiding this comment.
map0 should always be synced (disabled?)
| opacity: '1' | ||
| } | ||
| }, | ||
| zIndex: 1 |
There was a problem hiding this comment.
this might be the same problem with LayerPanel hiding
| // SplitScreen | ||
| '_ol_kit.SplitScreen.disabled': 'Disabled', | ||
| '_ol_kit.SplitScreen.addMap': 'Add Map', | ||
| '_ol_kit.SplitScreen.removeMap': 'Remove Map' |
stazrad
added
ready to ship 🚀
Resolves: attach_gh_issue_here
PR Safety Checklist:
package.json&package-lock.jsonversion numbers to appropriate releasenpm run docsQuick Description of Changes (+ screenshots for ui changes):