rusackas commented on code in PR #35867:
URL: https://github.com/apache/superset/pull/35867#discussion_r3651982101
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts:
##########
@@ -89,6 +89,11 @@ export class OptionSelector {
[this.values[a], this.values[b]] = [this.values[b], this.values[a]];
}
+ reorder(a: number, b: number) {
+ const [moved] = this.values.splice(a, 1);
+ this.values.splice(b, 0, moved);
+ }
Review Comment:
Good catch, fixed. `reorder` now bails on out-of-range or no-op indices
before it splices.
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts:
##########
@@ -89,6 +89,11 @@ export class OptionSelector {
[this.values[a], this.values[b]] = [this.values[b], this.values[a]];
}
+ reorder(from: number, to: number) {
+ const [moved] = this.values.splice(from, 1);
+ this.values.splice(to, 0, moved);
+ }
Review Comment:
Same fix as Korbit's note on the same method, `reorder` now guards
out-of-range and no-op indices before splicing.
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx:
##########
@@ -96,7 +96,7 @@ function DndColumnSelect(props: DndColumnSelectProps) {
const onShiftOptions = useCallback(
(dragIndex: number, hoverIndex: number) => {
Review Comment:
Went with the guard living inside `OptionSelector.reorder` itself instead of
duplicating it at every call site, same effect but one place to keep in sync.
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx:
##########
@@ -252,12 +252,19 @@ const DndFilterSelect = (props: DndFilterSelectProps) => {
const onShiftOptions = useCallback(
(dragIndex: number, hoverIndex: number) => {
- const newValues = [...values];
- [newValues[hoverIndex], newValues[dragIndex]] = [
- newValues[dragIndex],
- newValues[hoverIndex],
- ];
- setValues(newValues);
+ if (
+ dragIndex === hoverIndex ||
+ dragIndex < 0 ||
+ hoverIndex < 0 ||
+ dragIndex >= values.length ||
+ hoverIndex >= values.length
+ ) {
+ return;
+ }
+ const newValue = [...values];
+ const [moved] = newValue.splice(dragIndex, 1);
+ newValue.splice(hoverIndex, 0, moved);
+ setValues(newValue);
},
[values],
Review Comment:
Good catch, fixed. `onShiftOptions` now calls `onChange` after committing
the reordered array.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
