Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make positions possibly negatives #5690

Merged
merged 2 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { useRecordBoardStates } from '@/object-record/record-board/hooks/interna
import { useRecordBoardSelection } from '@/object-record/record-board/hooks/useRecordBoardSelection';
import { RecordBoardColumn } from '@/object-record/record-board/record-board-column/components/RecordBoardColumn';
import { RecordBoardScope } from '@/object-record/record-board/scopes/RecordBoardScope';
import { getDraggedRecordPosition } from '@/object-record/record-board/utils/get-dragged-record-position.util';
import { recordStoreFamilyState } from '@/object-record/record-store/states/recordStoreFamilyState';
import { TableHotkeyScope } from '@/object-record/record-table/types/TableHotkeyScope';
import { DragSelect } from '@/ui/utilities/drag-select/components/DragSelect';
Expand Down Expand Up @@ -107,7 +108,6 @@ export const RecordBoard = ({ recordBoardId }: RecordBoardProps) => {
.getLoadable(recordStoreFamilyState(recordBeforeId))
.getValue()
: null;
const recordBeforePosition: number | undefined = recordBefore?.position;

const recordAfterId =
otherRecordsInDestinationColumn[destinationIndexInColumn];
Expand All @@ -116,12 +116,11 @@ export const RecordBoard = ({ recordBoardId }: RecordBoardProps) => {
.getLoadable(recordStoreFamilyState(recordAfterId))
.getValue()
: null;
const recordAfterPosition: number | undefined = recordAfter?.position;

const beforeBoundary = recordBeforePosition ?? 0;
const afterBoundary = recordAfterPosition ?? beforeBoundary + 1;

const draggedRecordPosition = (beforeBoundary + afterBoundary) / 2;
const draggedRecordPosition = getDraggedRecordPosition(
recordBefore?.position,
recordAfter?.position,
);

updateOneRecord({
idToUpdate: draggedRecordId,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { getDraggedRecordPosition } from '../get-dragged-record-position.util';

describe('getDraggedRecordPosition', () => {
it('should return the average of the two positions', () => {
expect(getDraggedRecordPosition(1, 3)).toBe(2);
});

it('should return the position before the record after', () => {
expect(getDraggedRecordPosition(undefined, 3)).toBe(2);
thomtrp marked this conversation as resolved.
Show resolved Hide resolved
});

it('should return the position after the record before', () => {
expect(getDraggedRecordPosition(1, undefined)).toBe(2);
thomtrp marked this conversation as resolved.
Show resolved Hide resolved
});

it('should return 1 if no positions are defined', () => {
expect(getDraggedRecordPosition(undefined, undefined)).toBe(1);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { isDefined } from '~/utils/isDefined';

export const getDraggedRecordPosition = (
recordBeforePosition?: number,
recordAfterPosition?: number,
): number => {
if (isDefined(recordAfterPosition) && isDefined(recordBeforePosition)) {
return (recordBeforePosition + recordAfterPosition) / 2;
thomtrp marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you return in your if, I think we can have something with less indentation and more readable like this. Wdyt?

export const getDraggedRecordPosition = (
  recordBeforePosition?: number,
  recordAfterPosition?: number,
): number => {
  if (isDefined(recordAfterPosition) && isDefined(recordBeforePosition)) {
    return (recordBeforePosition + recordAfterPosition) / 2;
  }

  if (isDefined(recordAfterPosition)) {
    return recordAfterPosition - 1;
  }

  if (isDefined(recordBeforePosition)) {
    return recordBeforePosition + 1;
  }

  return 1;
};

} else if (isDefined(recordAfterPosition)) {
return recordAfterPosition - 1;
} else if (isDefined(recordBeforePosition)) {
return recordBeforePosition + 1;
} else {
return 1;
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ describe('RecordPositionFactory', () => {

expect(result).toEqual(value);
});
it('should return the existing position / 2 when value is first', async () => {
it('should return the existing position -1 when value is first', async () => {
const value = 'first';
const result = await factory.create(value, objectMetadata, workspaceId);

expect(result).toEqual(0.5);
expect(result).toEqual(0);
});
it('should return the existing position + 1 when value is last', async () => {
const value = 'last';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { Injectable } from '@nestjs/common';

import { isDefined } from 'class-validator';

import { WorkspaceDataSourceService } from 'src/engine/workspace-datasource/workspace-datasource.service';
import {
RecordPositionQueryFactory,
Expand Down Expand Up @@ -32,17 +34,25 @@ export class RecordPositionFactory {
dataSourceSchema,
);

// If the value was 'first', the first record will be the one with the lowest position
// If the value was 'last', the first record will be the one with the highest position
const records = await this.workspaceDataSourceService.executeRawQuery(
query,
[],
workspaceId,
undefined,
);

return (
(value === 'first'
? records[0]?.position / 2
: records[0]?.position + 1) || 1
);
if (
!isDefined(records) ||
records.length === 0 ||
!isDefined(records[0]?.position)
) {
return 1;
}

return value === 'first'
? records[0].position - 1
: records[0].position + 1;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const isValidStringPosition = (value: string): boolean =>
typeof value === 'string' && (value === 'first' || value === 'last');

const isValidNumberPosition = (value: number): boolean =>
typeof value === 'number' && value >= 0;
typeof value === 'number';

const checkPosition = (value: any): PositionType => {
if (isValidNumberPosition(value) || isValidStringPosition(value)) {
Expand Down
Loading