From 74b47e7e744561d5875af9a234ba1c2f3f08f49b Mon Sep 17 00:00:00 2001 From: orangemug Date: Sat, 28 Mar 2020 10:58:47 +0000 Subject: [PATCH 1/4] 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)} />
    • } From 6f83839a4cdb635b334863dc2dd2e8ba17b6fa67 Mon Sep 17 00:00:00 2001 From: orangemug Date: Sat, 28 Mar 2020 11:06:45 +0000 Subject: [PATCH 2/4] Added missing file. --- src/components/util/format.js | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 src/components/util/format.js diff --git a/src/components/util/format.js b/src/components/util/format.js new file mode 100644 index 0000000..44e494d --- /dev/null +++ b/src/components/util/format.js @@ -0,0 +1,3 @@ +export function formatLayerId (id) { + return id === "" ? "[empty_string]" : `'${id}'`; +} From 61ba399e1c3804531e634f3405dcf9af45f172ab Mon Sep 17 00:00:00 2001 From: orangemug Date: Mon, 30 Mar 2020 09:47:46 +0100 Subject: [PATCH 3/4] Duplicate layer ids now show errors inline. --- src/components/App.jsx | 36 +++++++++++++++++--------- src/components/MessagePanel.jsx | 6 +++-- src/components/layers/LayerEditor.jsx | 3 ++- src/components/layers/LayerIdBlock.jsx | 1 + 4 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/components/App.jsx b/src/components/App.jsx index d1e3902..aabc06d 100644 --- a/src/components/App.jsx +++ b/src/components/App.jsx @@ -335,22 +335,33 @@ export default class App extends React.Component { 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, - } - } - }); + const error = new Error( + `layers[${index}]: duplicate layer id [empty_string], previously used` + ); + layerErrors.push(error); } foundLayers.set(layer.id, true); }); } - const mappedErrors = errors.map(error => { + const mappedErrors = layerErrors.concat(errors).map(error => { + const dupMatch = error.message.match(/layers\[(\d+)\]: (duplicate layer id "?(.*)"?, previously used)/); + if (dupMatch) { + const [matchStr, index, message] = dupMatch; + return { + message: error.message, + parsed: { + type: "layer", + data: { + index, + key: "id", + message, + } + } + } + } + + // duplicate layer id const layerMatch = error.message.match(/layers\[(\d+)\]\.(?:(\S+)\.)?(\S+): (.*)/); if (layerMatch) { const [matchStr, index, group, property, message] = layerMatch; @@ -372,7 +383,7 @@ export default class App extends React.Component { message: error.message, }; } - }).concat(layerErrors); + }); let dirtyMapStyle = undefined; if (errors.length > 0) { @@ -753,6 +764,7 @@ export default class App extends React.Component { const bottomPanel = (this.state.errors.length + this.state.infos.length) > 0 ? { let content; if (error.parsed && error.parsed.type === "layer") { @@ -25,12 +27,12 @@ class MessagePanel extends React.Component { content = ( <> Layer {formatLayerId(layerId)}: {parsed.data.message} - {currentLayer.id !== layerId && + {selectedLayerIndex !== parsed.data.index && <>  —  diff --git a/src/components/layers/LayerEditor.jsx b/src/components/layers/LayerEditor.jsx index bc0150e..7ee3ff4 100644 --- a/src/components/layers/LayerEditor.jsx +++ b/src/components/layers/LayerEditor.jsx @@ -19,6 +19,7 @@ import {MdMoreVert} from 'react-icons/md' import { changeType, changeProperty } from '../../libs/layer' import layout from '../../config/layout.json' +import {formatLayerId} from '../util/format'; function getLayoutForType (type) { @@ -292,7 +293,7 @@ export default class LayerEditor extends React.Component {

    - Layer: {this.props.layer.id} + Layer: {formatLayerId(this.props.layer.id)}

    Date: Mon, 30 Mar 2020 09:57:14 +0100 Subject: [PATCH 4/4] Fixed lint errors. --- package.json | 5 +++++ src/components/layers/LayerIdBlock.jsx | 1 + 2 files changed, 6 insertions(+) diff --git a/package.json b/package.json index 540436e..5297be2 100644 --- a/package.json +++ b/package.json @@ -102,6 +102,11 @@ "experimentalObjectRestSpread": true, "jsx": true } + }, + "settings": { + "react": { + "version": "detect" + } } }, "devDependencies": { diff --git a/src/components/layers/LayerIdBlock.jsx b/src/components/layers/LayerIdBlock.jsx index 98321a5..120378d 100644 --- a/src/components/layers/LayerIdBlock.jsx +++ b/src/components/layers/LayerIdBlock.jsx @@ -10,6 +10,7 @@ class LayerIdBlock extends React.Component { value: PropTypes.string.isRequired, wdKey: PropTypes.string.isRequired, onChange: PropTypes.func.isRequired, + error: PropTypes.object.isRequired, } render() {