From 74b47e7e744561d5875af9a234ba1c2f3f08f49b Mon Sep 17 00:00:00 2001 From: orangemug Date: Sat, 28 Mar 2020 10:58:47 +0000 Subject: [PATCH] Fix UI when loading invalid style with duplicate layer ids. Also fix for throwing error when 2 layers exist with empty strings as ids. --- src/components/App.jsx | 64 ++++++++++++++++--------- src/components/MessagePanel.jsx | 3 +- src/components/inputs/StringInput.jsx | 5 ++ src/components/layers/LayerEditor.jsx | 25 +++++++--- src/components/layers/LayerList.jsx | 14 ++++-- src/components/layers/LayerListItem.jsx | 9 ++-- 6 files changed, 82 insertions(+), 38 deletions(-) diff --git a/src/components/App.jsx b/src/components/App.jsx index 839129d..d1e3902 100644 --- a/src/components/App.jsx +++ b/src/components/App.jsx @@ -36,6 +36,7 @@ import tokens from '../config/tokens.json' import isEqual from 'lodash.isequal' import Debug from '../libs/debug' import queryUtil from '../libs/query-util' +import {formatLayerId} from './util/format'; import MapboxGl from 'mapbox-gl' @@ -325,6 +326,30 @@ export default class App extends React.Component { }; const errors = validate(newStyle, latest) || []; + + // The validate function doesn't give us errors for duplicate error with + // empty string for layer.id, manually deal with that here. + const layerErrors = []; + if (newStyle && newStyle.layers) { + const foundLayers = new Map(); + newStyle.layers.forEach((layer, index) => { + if (layer.id === "" && foundLayers.has(layer.id)) { + const message = `Duplicate layer: ${formatLayerId(layer.id)}`; + layerErrors.push({ + message, + parsed: { + type: "layer", + data: { + index, + message, + } + } + }); + } + foundLayers.set(layer.id, true); + }); + } + const mappedErrors = errors.map(error => { const layerMatch = error.message.match(/layers\[(\d+)\]\.(?:(\S+)\.)?(\S+): (.*)/); if (layerMatch) { @@ -347,7 +372,7 @@ export default class App extends React.Component { message: error.message, }; } - }) + }).concat(layerErrors); let dirtyMapStyle = undefined; if (errors.length > 0) { @@ -437,56 +462,50 @@ export default class App extends React.Component { this.onStyleChanged(changedStyle) } - onLayerDestroy = (layerId) => { + onLayerDestroy = (index) => { let layers = this.state.mapStyle.layers; const remainingLayers = layers.slice(0); - const idx = style.indexOfLayer(remainingLayers, layerId) - remainingLayers.splice(idx, 1); + remainingLayers.splice(index, 1); this.onLayersChange(remainingLayers); } - onLayerCopy = (layerId) => { + onLayerCopy = (index) => { let layers = this.state.mapStyle.layers; const changedLayers = layers.slice(0) - const idx = style.indexOfLayer(changedLayers, layerId) - const clonedLayer = cloneDeep(changedLayers[idx]) + const clonedLayer = cloneDeep(changedLayers[index]) clonedLayer.id = clonedLayer.id + "-copy" - changedLayers.splice(idx, 0, clonedLayer) + changedLayers.splice(index, 0, clonedLayer) this.onLayersChange(changedLayers) } - onLayerVisibilityToggle = (layerId) => { + onLayerVisibilityToggle = (index) => { let layers = this.state.mapStyle.layers; const changedLayers = layers.slice(0) - const idx = style.indexOfLayer(changedLayers, layerId) - const layer = { ...changedLayers[idx] } + const layer = { ...changedLayers[index] } const changedLayout = 'layout' in layer ? {...layer.layout} : {} changedLayout.visibility = changedLayout.visibility === 'none' ? 'visible' : 'none' layer.layout = changedLayout - changedLayers[idx] = layer + changedLayers[index] = layer this.onLayersChange(changedLayers) } - onLayerIdChange = (oldId, newId) => { + onLayerIdChange = (index, oldId, newId) => { const changedLayers = this.state.mapStyle.layers.slice(0) - const idx = style.indexOfLayer(changedLayers, oldId) - - changedLayers[idx] = { - ...changedLayers[idx], + changedLayers[index] = { + ...changedLayers[index], id: newId } this.onLayersChange(changedLayers) } - onLayerChanged = (layer) => { + onLayerChanged = (index, layer) => { const changedLayers = this.state.mapStyle.layers.slice(0) - const idx = style.indexOfLayer(changedLayers, layer.id) - changedLayers[idx] = layer + changedLayers[index] = layer this.onLayersChange(changedLayers) } @@ -645,9 +664,8 @@ export default class App extends React.Component { } - onLayerSelect = (layerId) => { - const idx = style.indexOfLayer(this.state.mapStyle.layers, layerId) - this.setState({ selectedLayerIndex: idx }) + onLayerSelect = (index) => { + this.setState({ selectedLayerIndex: index }) } setModal(modalName, value) { diff --git a/src/components/MessagePanel.jsx b/src/components/MessagePanel.jsx index f114a8c..105eeb3 100644 --- a/src/components/MessagePanel.jsx +++ b/src/components/MessagePanel.jsx @@ -1,5 +1,6 @@ import React from 'react' import PropTypes from 'prop-types' +import {formatLayerId} from './util/format'; class MessagePanel extends React.Component { static propTypes = { @@ -23,7 +24,7 @@ class MessagePanel extends React.Component { const layerId = mapStyle.layers[parsed.data.index].id; content = ( <> - Layer '{layerId}': {parsed.data.message} + Layer {formatLayerId(layerId)}: {parsed.data.message} {currentLayer.id !== layerId && <>  —  diff --git a/src/components/inputs/StringInput.jsx b/src/components/inputs/StringInput.jsx index c45e02c..5c6fa58 100644 --- a/src/components/inputs/StringInput.jsx +++ b/src/components/inputs/StringInput.jsx @@ -79,6 +79,11 @@ class StringInput extends React.Component { this.props.onChange(this.state.value); } }, + onKeyDown: (e) => { + if (e.keyCode === 13) { + this.props.onChange(this.state.value); + } + }, required: this.props.required, }); } diff --git a/src/components/layers/LayerEditor.jsx b/src/components/layers/LayerEditor.jsx index 79b4c16..291cae2 100644 --- a/src/components/layers/LayerEditor.jsx +++ b/src/components/layers/LayerEditor.jsx @@ -107,7 +107,10 @@ export default class LayerEditor extends React.Component { } changeProperty(group, property, newValue) { - this.props.onLayerChanged(changeProperty(this.props.layer, group, property, newValue)) + this.props.onLayerChanged( + this.props.layerIndex, + changeProperty(this.props.layer, group, property, newValue) + ) } onGroupToggle(groupTitle, active) { @@ -151,13 +154,16 @@ export default class LayerEditor extends React.Component { value={this.props.layer.id} wdKey="layer-editor.layer-id" error={errorData.id} - onChange={newId => this.props.onLayerIdChange(this.props.layer.id, newId)} + onChange={newId => this.props.onLayerIdChange(this.props.layerIndex, this.props.layer.id, newId)} /> this.props.onLayerChanged(changeType(this.props.layer, newType))} + onChange={newType => this.props.onLayerChanged( + this.props.layerIndex, + changeType(this.props.layer, newType) + )} /> {this.props.layer.type !== 'background' && case 'jsoneditor': return { + this.props.onLayerChanged( + this.props.layerIndex, + layer + ); + }} /> } } @@ -242,15 +253,15 @@ export default class LayerEditor extends React.Component { const items = { delete: { text: "Delete", - handler: () => this.props.onLayerDestroy(this.props.layer.id) + handler: () => this.props.onLayerDestroy(this.props.layerIndex) }, duplicate: { text: "Duplicate", - handler: () => this.props.onLayerCopy(this.props.layer.id) + handler: () => this.props.onLayerCopy(this.props.layerIndex) }, hide: { text: (layout.visibility === "none") ? "Show" : "Hide", - handler: () => this.props.onLayerVisibilityToggle(this.props.layer.id) + handler: () => this.props.onLayerVisibilityToggle(this.props.layerIndex) }, moveLayerUp: { text: "Move layer up", diff --git a/src/components/layers/LayerList.jsx b/src/components/layers/LayerList.jsx index 8d60bb4..12c4044 100644 --- a/src/components/layers/LayerList.jsx +++ b/src/components/layers/LayerList.jsx @@ -165,12 +165,15 @@ class LayerListContainer extends React.Component { const listItems = [] let idx = 0 - this.groupedLayers().forEach(layers => { + const layerIdCount = new Map(); + + const layersByGroup = this.groupedLayers(); + layersByGroup.forEach(layers => { const groupPrefix = layerPrefix(layers[0].id) if(layers.length > 1) { const grp = 1 && this.isCollapsed(groupPrefix, groupIdx) && idx !== this.props.selectedLayerIndex, @@ -196,8 +203,9 @@ class LayerListContainer extends React.Component { 'maputnik-layer-list-item--error': !!layerError })} index={idx} - key={layer.id} + key={key} layerId={layer.id} + layerIndex={idx} layerType={layer.type} visibility={(layer.layout || {}).visibility} isSelected={idx === this.props.selectedLayerIndex} diff --git a/src/components/layers/LayerListItem.jsx b/src/components/layers/LayerListItem.jsx index 0e9a238..7f1719e 100644 --- a/src/components/layers/LayerListItem.jsx +++ b/src/components/layers/LayerListItem.jsx @@ -62,6 +62,7 @@ class IconAction extends React.Component { class LayerListItem extends React.Component { static propTypes = { + layerIndex: PropTypes.number.isRequired, layerId: PropTypes.string.isRequired, layerType: PropTypes.string.isRequired, isSelected: PropTypes.bool, @@ -97,7 +98,7 @@ class LayerListItem extends React.Component { return
  • this.props.onLayerSelect(this.props.layerId)} + onClick={e => this.props.onLayerSelect(this.props.layerIndex)} data-wd-key={"layer-list-item:"+this.props.layerId} className={classnames({ "maputnik-layer-list-item": true, @@ -110,20 +111,20 @@ class LayerListItem extends React.Component { wdKey={"layer-list-item:"+this.props.layerId+":delete"} action={'delete'} classBlockName="delete" - onClick={e => this.props.onLayerDestroy(this.props.layerId)} + onClick={e => this.props.onLayerDestroy(this.props.layerIndex)} /> this.props.onLayerCopy(this.props.layerId)} + onClick={e => this.props.onLayerCopy(this.props.layerIndex)} /> this.props.onLayerVisibilityToggle(this.props.layerId)} + onClick={e => this.props.onLayerVisibilityToggle(this.props.layerIndex)} />
    • }