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

[Issue-5772] Add sort feature on settings tables #5787

Merged
merged 21 commits into from
Aug 14, 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
105 changes: 105 additions & 0 deletions packages/twenty-front/src/hooks/__tests__/useTableSort.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import { act, renderHook } from '@testing-library/react';

import {
mockedTableData as tableData,
tableDataSortedByFieldsCountInAscendingOrder,
tableDataSortedByFieldsCountInDescendingOrder,
tableDataSortedBylabelInAscendingOrder,
tableDataSortedBylabelInDescendingOrder,
} from '~/testing/mock-data/tableData';
Anand-Krishnan-M-J marked this conversation as resolved.
Show resolved Hide resolved

import { useTableSort } from '../useTableSort';

describe('useTableSort hook', () => {
test('initial sorting behavior for string fields', () => {
const { result } = renderHook(() => useTableSort('labelPlural', tableData));
const [sortedData, , sortConfig] = result.current;

expect(JSON.stringify(sortedData)).toBe(
JSON.stringify(tableDataSortedBylabelInAscendingOrder),
);
expect(sortConfig).toEqual({
sortByColumnKey: 'labelPlural',
sortOrder: 'ascending',
});
});

test('initial sorting behavior for number fields', () => {
const { result } = renderHook(() => useTableSort('fieldsCount', tableData));
const [sortedData, , sortConfig] = result.current;

expect(JSON.stringify(sortedData)).toBe(
JSON.stringify(tableDataSortedByFieldsCountInAscendingOrder),
);
expect(sortConfig).toEqual({
sortByColumnKey: 'fieldsCount',
sortOrder: 'ascending',
});
});

test('sorting behavior when clicking on string column header', () => {
const { result } = renderHook(() => useTableSort('fieldsCount', tableData));
const [, handleSortClick] = result.current;

act(() => {
handleSortClick('labelPlural');
});

let [sortedData, , sortConfig] = result.current;

expect(JSON.stringify(sortedData)).toBe(
JSON.stringify(tableDataSortedBylabelInAscendingOrder),
);
expect(sortConfig).toEqual({
sortByColumnKey: 'labelPlural',
sortOrder: 'ascending',
});

act(() => {
handleSortClick('labelPlural');
});

[sortedData, , sortConfig] = result.current;

expect(JSON.stringify(sortedData)).toBe(
JSON.stringify(tableDataSortedBylabelInDescendingOrder),
);
expect(sortConfig).toEqual({
sortByColumnKey: 'labelPlural',
sortOrder: 'descending',
});
});

test('sorting behavior when clicking on number column header', () => {
const { result } = renderHook(() => useTableSort('labelPlural', tableData));
const [, handleSortClick] = result.current;

act(() => {
handleSortClick('fieldsCount');
});

let [sortedData, , sortConfig] = result.current;

expect(JSON.stringify(sortedData)).toBe(
JSON.stringify(tableDataSortedByFieldsCountInAscendingOrder),
);
expect(sortConfig).toEqual({
sortByColumnKey: 'fieldsCount',
sortOrder: 'ascending',
});

act(() => {
handleSortClick('fieldsCount');
});

[sortedData, , sortConfig] = result.current;

expect(JSON.stringify(sortedData)).toBe(
JSON.stringify(tableDataSortedByFieldsCountInDescendingOrder),
);
expect(sortConfig).toEqual({
sortByColumnKey: 'fieldsCount',
sortOrder: 'descending',
});
});
});
62 changes: 62 additions & 0 deletions packages/twenty-front/src/hooks/useTableSort.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import { useState } from 'react';

enum sortOrders {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have OrderBy

Copy link
Contributor Author

@Anand-Krishnan-M-J Anand-Krishnan-M-J Jul 4, 2024

Choose a reason for hiding this comment

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

Updated the type to OrderBy. thanks for pointing it out.

ascending = 'ascending',
descending = 'descending',
}
type SortConfig<T> = {
sortByColumnKey: keyof T;
sortOrder: sortOrders;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming sortOrders to SortOrders to follow TypeScript enum naming conventions.


export const useTableSort = <T>(
initialColumnKey: keyof T,
tableData: T[],
): [T[], (sortKey: keyof T) => void, SortConfig<T>] => {
const [sortConfig, setSortConfig] = useState<SortConfig<T>>({
sortByColumnKey: initialColumnKey,
sortOrder: sortOrders.ascending,
});

const sortTableData = (
data: T[],
columnKey: keyof T,
order: sortOrders,
): T[] => {
return data.sort((a: T, b: T) => {
if (typeof a[columnKey] === 'string') {
return order === sortOrders.ascending
? (a[columnKey] as string).localeCompare(b[columnKey] as string)
: (b[columnKey] as string).localeCompare(a[columnKey] as string);
} else if (typeof a[columnKey] === 'number') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Handle cases where a[columnKey] or b[columnKey] might be undefined to prevent runtime errors.

return order === sortOrders.ascending
? (a[columnKey] as number) - (b[columnKey] as number)
: (b[columnKey] as number) - (a[columnKey] as number);
} else return 0;
});
};

const sortedTableData = sortTableData(
[...tableData],
sortConfig.sortByColumnKey,
sortConfig.sortOrder,
);

const toggleSortOrder = (sortOrder: sortOrders) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function isn't toggling, it's just returning the opposite of the passed sortOrder, so it should either be renamed or either really toggling the state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the logic is moved to component, we no longer have toggleSortOrder()

return sortOrder === sortOrders.ascending
? sortOrders.descending
: sortOrders.ascending;
};

const handleSortClick = (columnKey: keyof T) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Handlers should be the scope of a component, not of a hook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the logic inside component

setSortConfig((state) => ({
sortByColumnKey: columnKey,
sortOrder:
state.sortByColumnKey === columnKey
? toggleSortOrder(state.sortOrder)
: sortOrders.ascending,
}));
};

return [sortedTableData, handleSortClick, sortConfig];
};
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ReactNode, useMemo } from 'react';
import { ReactNode, useEffect, useMemo } from 'react';
import { useTheme } from '@emotion/react';
import styled from '@emotion/styled';
import { Nullable, useIcons } from 'twenty-ui';
Expand All @@ -7,6 +7,7 @@ import { useGetRelationMetadata } from '@/object-metadata/hooks/useGetRelationMe
import { FieldMetadataItem } from '@/object-metadata/types/FieldMetadataItem';
import { getObjectSlug } from '@/object-metadata/utils/getObjectSlug';
import { FieldIdentifierType } from '@/settings/data-model/types/FieldIdentifierType';
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure fieldType is correctly populated in all instances of fieldMetadataItem.

import { getSettingsFieldTypeConfig } from '@/settings/data-model/utils/getSettingsFieldTypeConfig';
import { isFieldTypeSupportedInSettings } from '@/settings/data-model/utils/isFieldTypeSupportedInSettings';
import { TableCell } from '@/ui/layout/table/components/TableCell';
import { TableRow } from '@/ui/layout/table/components/TableRow';
Expand All @@ -17,11 +18,12 @@ import { SettingsObjectFieldDataType } from './SettingsObjectFieldDataType';

type SettingsObjectFieldItemTableRowProps = {
ActionIcon: ReactNode;
fieldMetadataItem: FieldMetadataItem;
fieldMetadataItem: FieldMetadataItem & { fieldType: string | boolean };
identifierType?: Nullable<FieldIdentifierType>;
Copy link
Contributor

Choose a reason for hiding this comment

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

fieldType is redundant, we already have a type property on fieldMetadataItem which should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the redundant field

variant?: 'field-type' | 'identifier';
isRemoteObjectField?: boolean;
to?: string;
updateDataTypes: (id: string, label?: string) => void;
};

export const StyledObjectFieldTableRow = styled(TableRow)`
Expand All @@ -41,10 +43,8 @@ const StyledIconTableCell = styled(TableCell)`
export const SettingsObjectFieldItemTableRow = ({
ActionIcon,
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a default value or error handling for updateDataTypes to avoid potential runtime errors.

fieldMetadataItem,
identifierType,
variant = 'field-type',
isRemoteObjectField,
to,
updateDataTypes,
}: SettingsObjectFieldItemTableRowProps) => {
const theme = useTheme();
const { getIcon } = useIcons();
Expand All @@ -60,6 +60,18 @@ export const SettingsObjectFieldItemTableRow = ({

const fieldType = fieldMetadataItem.type;
const isFieldTypeSupported = isFieldTypeSupportedInSettings(fieldType);
useEffect(() => {
const fieldTypeConfig = getSettingsFieldTypeConfig(fieldMetadataItem.type);
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is not necessary because we already have a fieldMetadataItem array higher up in the tree, the row shouldn't be related to this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the unwanted logic and used the actual value in higher level component instead

const label =
relationObjectMetadataItem?.labelPlural ?? fieldTypeConfig?.label;
//Hoist data types to parent component state to determine sort order
updateDataTypes(fieldMetadataItem.id, label);
}, [
fieldMetadataItem.id,
fieldMetadataItem.type,
relationObjectMetadataItem,
updateDataTypes,
]);

if (!isFieldTypeSupported) return null;

Expand All @@ -75,17 +87,7 @@ export const SettingsObjectFieldItemTableRow = ({
)}
{fieldMetadataItem.label}
</StyledNameTableCell>
<TableCell>
{variant === 'field-type' &&
(isRemoteObjectField
? 'Remote'
: fieldMetadataItem.isCustom
? 'Custom'
: 'Standard')}
{variant === 'identifier' &&
!!identifierType &&
(identifierType === 'label' ? 'Record text' : 'Record image')}
</TableCell>
<TableCell>{fieldMetadataItem.fieldType}</TableCell>
<TableCell>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be keep the derived state here, not adding a new field on fieldMetadataItem, if you need to share it with another place just create a util like getFieldMetadataItemTypeLabel(fieldMetadataItem)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with the derived state

<SettingsObjectFieldDataType
Icon={RelationIcon}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,27 @@
import { ReactNode } from 'react';
import { ReactNode, useEffect } from 'react';
import { useTheme } from '@emotion/react';
import styled from '@emotion/styled';
import { useIcons } from 'twenty-ui';

import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
import { useFindManyRecords } from '@/object-record/hooks/useFindManyRecords';
import { SettingsDataModelObjectTypeTag } from '@/settings/data-model/objects/SettingsDataModelObjectTypeTag';
import { getObjectTypeLabel } from '@/settings/data-model/utils/getObjectTypeLabel';
import {
getObjectTypeLabel,
ObjectTypeLabel,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure useEffect dependency array is correct to avoid unnecessary re-renders.

} from '@/settings/data-model/utils/getObjectTypeLabel';
import { TableCell } from '@/ui/layout/table/components/TableCell';
import { TableRow } from '@/ui/layout/table/components/TableRow';

type SettingsObjectItemTableRowProps = {
export type SettingsObjectItemTableRowProps = {
action: ReactNode;
objectItem: ObjectMetadataItem;
objectItem: ObjectMetadataItem & {
objectTypeLabelText: ObjectTypeLabel['labelText'];
fieldsCount: number;
instancesCount: number;
};
to?: string;
updateInstanceCount: (id: string, instanceCount: number) => void;
};

export const StyledObjectTableRow = styled(TableRow)`
Expand All @@ -34,12 +42,18 @@ export const SettingsObjectItemTableRow = ({
action,
objectItem,
to,
updateInstanceCount,
}: SettingsObjectItemTableRowProps) => {
const theme = useTheme();

const { totalCount } = useFindManyRecords({
objectNameSingular: objectItem.nameSingular,
});
useEffect(() => {
//Hoist instance count to parent component state to determine sort order
Copy link
Member

Choose a reason for hiding this comment

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

Can you try getting rid of useEffect? https://react.dev/learn/you-might-not-need-an-effect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me find a way to do that. Probably I will have to create a global state to do that, as we have to store the data somewhere to actually use it higher up the React tree.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use a recoil family here also

updateInstanceCount(objectItem.id, totalCount || 0);
}, [objectItem.id, totalCount, updateInstanceCount]);

const { getIcon } = useIcons();
const Icon = getIcon(objectItem.icon);
const objectTypeLabel = getObjectTypeLabel(objectItem);
Expand All @@ -55,9 +69,7 @@ export const SettingsObjectItemTableRow = ({
<TableCell>
<SettingsDataModelObjectTypeTag objectTypeLabel={objectTypeLabel} />
</TableCell>
<TableCell align="right">
{objectItem.fields.filter((field) => !field.isSystem).length}
</TableCell>
<TableCell align="right">{objectItem.fieldsCount}</TableCell>
<TableCell align="right">{totalCount}</TableCell>
<StyledActionTableCell>{action}</StyledActionTableCell>
</StyledObjectTableRow>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import styled from '@emotion/styled';

const StyledTableHeader = styled.div<{ align?: 'left' | 'center' | 'right' }>`
const StyledTableHeader = styled.div<{
align?: 'left' | 'center' | 'right';
onClick?: () => void;
}>`
align-items: center;
border-bottom: 1px solid ${({ theme }) => theme.border.color.light};
color: ${({ theme }) => theme.font.color.tertiary};
Expand All @@ -15,6 +18,7 @@ const StyledTableHeader = styled.div<{ align?: 'left' | 'center' | 'right' }>`
: 'flex-start'};
padding: 0 ${({ theme }) => theme.spacing(2)};
text-align: ${({ align }) => align ?? 'left'};
cursor: ${({ onClick }) => (onClick ? 'pointer' : 'default')};
`;

export { StyledTableHeader as TableHeader };
Loading
Loading