Layer selection #166
Layer selection #166
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @akshay1992kalbhor)
client/web/.DS_Store, line 0 at r1 (raw file):
Please remove
client/web/src/components/panels/LayerTree.vue, line 21 at r1 (raw file):
<div class="layer" :class="layer.selected ? 'selected' : ''"
For purely stylistic reasons, could you please replace this with:
:class="{selected: layer.selected}"
client/web/src/components/panels/LayerTree.vue, line 135 at r1 (raw file):
break; } j += 1;
j++;
client/web/src/components/panels/LayerTree.vue, line 155 at r1 (raw file):
} } i += 1;
i++;
client/web/src/components/panels/LayerTree.vue, line 210 at r1 (raw file):
SeparatorType, layers: [] as Array<LayerPanelEntry>, startPath: -1n,
A clearer name might be selectionRangeStartLayer and selectionRangeEndLayer. It should probably be of type LayerPanelEntry | undefined (so in TypeScript I believe this line would say selectionRangeStartLayer: undefined as LayerPanelEntry | undefined,). You will store a reference to the layer (not only the path) that's also in the layers array (see the line above this comment). Then for the range selection, the algorithm can find where the selectionRangeStartLayer and selectionRangeEndLayer are within the array (and eventually within the tree once we have folders) using [a, b, c].indexOf(b).
I will leave more detailed feedback on your three click selection functions once you switch over the data type.
Your JavaScript/TypeScript is looking seriously awesome despite your minimal experience using it!
TrueDoctor
force-pushed
the
layer-selection
branch
from
154a741
to
8de0517
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @akshay1992kalbhor and @Keavon)
client/web/.DS_Store, line at r1 (raw file):
Previously, Keavon (Keavon Chambers) wrote…
Please remove
Done.
client/web/src/components/panels/LayerTree.vue, line 21 at r1 (raw file):
Previously, Keavon (Keavon Chambers) wrote…
For purely stylistic reasons, could you please replace this with:
:class="{selected: layer.selected}"
Done.
client/web/src/components/panels/LayerTree.vue, line 135 at r1 (raw file):
Previously, Keavon (Keavon Chambers) wrote…
j++;
eslint complains about that, kinda justified
client/web/src/components/panels/LayerTree.vue, line 210 at r1 (raw file):
Previously, Keavon (Keavon Chambers) wrote…
A clearer name might be
selectionRangeStartLayerandselectionRangeEndLayer. It should probably be of typeLayerPanelEntry | undefined(so in TypeScript I believe this line would sayselectionRangeStartLayer: undefined as LayerPanelEntry | undefined,). You will store a reference to the layer (not only the path) that's also in thelayersarray (see the line above this comment). Then for the range selection, the algorithm can find where theselectionRangeStartLayerandselectionRangeEndLayerare within the array (and eventually within the tree once we have folders) using[a, b, c].indexOf(b).I will leave more detailed feedback on your three click selection functions once you switch over the data type.
Your JavaScript/TypeScript is looking seriously awesome despite your minimal experience using it!
Done.
| async updateSelection() { | ||
| const paths = this.layers.filter((layer) => layer.layer_data.selected).map((layer) => layer.path); | ||
| const length = paths.reduce((acc, cur) => acc + cur.length, 0) + paths.length - 1; | ||
| const output = new BigUint64Array(length); |
There was a problem hiding this comment.
We apparently need CI to run a newer version of Node.js for this. rustwasm/wasm-bindgen#967
There was a problem hiding this comment.
Reviewed 2 of 3 files at r3, 1 of 1 files at r4, 1 of 2 files at r5.
Reviewable status: 3 of 4 files reviewed, 8 unresolved discussions (waiting on @akshay1992kalbhor, @Keavon, and @TrueDoctor)
client/web/src/components/panels/LayerTree.vue, line 175 at r3 (raw file):
Previously, Keavon (Keavon Chambers) wrote…
We apparently need CI to run a newer version of Node.js for this. rustwasm/wasm-bindgen#967
Done.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r5.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @akshay1992kalbhor and @Keavon)
There was a problem hiding this comment.
Reviewed 2 of 3 files at r3, 2 of 2 files at r5.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @akshay1992kalbhor and @Keavon)
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @akshay1992kalbhor)
* Change arg to IconButton component * Add basic layer selection mechanism * Clean up print statements and add some comments * Simplified the layer selection mechanism * Remove redundant rule for 'no-param-reassign' * Clean up frontend and and plumb selection flow to backend * Update eslintrc Co-authored-by: Dennis Kobert <dennis@kobert.dev>
Very crude version. This is my first time writing JS seriously
This change is