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

Anand-Krishnan-M-J · 2024-06-09T16:26:20Z

Proposed Changes

Introduce a new custom hook - useTableSort to sort table content
Add test cases for the new custom hook
Integrate useTableSort hook on to the table in settings object and settings object field pages

Related Issue

#5772

Evidence

2024-06-09.21-35-41.mp4

Further comments

Apologies for the large PR. Looking forward for the review

greptile-apps

PR Summary

Introduced useTableSort custom hook for table sorting
Added test cases for useTableSort
Integrated sorting in SettingsObjectFieldItemTableRow and SettingsObjectItemTableRow
Enabled clickable headers for sorting in TableHeader
Updated SettingsObjectDetail and SettingsObjects pages to use sorting functionality

greptile-apps · 2024-06-09T16:26:44Z

packages/twenty-front/src/hooks/__tests__/useTableSort.test.ts

+  tableDataSortedByFieldsCountInDescendingOrder,
+  tableDataSortedBylabelInAscendingOrder,
+  tableDataSortedBylabelInDescendingOrder,
+} from '~/testing/mock-data/tableData';


Use toEqual instead of toBe for comparing objects or arrays to avoid potential issues with reference equality.

greptile-apps · 2024-06-09T16:27:00Z

packages/twenty-front/src/hooks/useTableSort.ts

+type SortConfig<T> = {
+  sortByColumnKey: keyof T;
+  sortOrder: sortOrders;
+};


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

greptile-apps · 2024-06-09T16:27:00Z

packages/twenty-front/src/hooks/useTableSort.ts

+        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') {


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

greptile-apps · 2024-06-09T16:27:20Z

...rc/modules/settings/data-model/object-details/components/SettingsObjectFieldItemTableRow.tsx

@@ -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';


Ensure fieldType is correctly populated in all instances of fieldMetadataItem.

greptile-apps · 2024-06-09T16:27:20Z

...rc/modules/settings/data-model/object-details/components/SettingsObjectFieldItemTableRow.tsx

@@ -41,10 +43,8 @@ const StyledIconTableCell = styled(TableCell)`
 export const SettingsObjectFieldItemTableRow = ({


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

greptile-apps · 2024-06-09T16:27:38Z

...ont/src/modules/settings/data-model/object-details/components/SettingsObjectItemTableRow.tsx

 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,


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

greptile-apps · 2024-06-09T16:28:42Z

packages/twenty-front/src/pages/settings/data-model/SettingsObjectDetail.tsx

@@ -1,7 +1,13 @@
-import { useEffect } from 'react';
+import { useCallback, useEffect, useState } from 'react';


Consider importing useCallback and useState from React in a single line for better readability.

greptile-apps · 2024-06-09T16:29:01Z

packages/twenty-front/src/pages/settings/data-model/SettingsObjects.tsx

@@ -38,13 +64,74 @@ const StyledH1Title = styled(H1Title)`
  margin-bottom: 0;


Ensure that instanceCountObj is correctly populated before using it in getRowData to avoid potential undefined values.

Anand-Krishnan-M-J · 2024-06-10T13:28:26Z

Hi Charles, Thanks a lot for your feedback. I will look in this and update the PR. Some immediate work has come up and I hope to fix this on the weekend.

…

On Mon, 10 Jun 2024, 5:11 pm Charles Bochet, ***@***.***> wrote: @Anand-Krishnan-M-J <https://github.com/Anand-Krishnan-M-J> Thanks for your PR :) Honnestly, I think we should restrict the scope to supporting Links filtering. Supporting sort in this PR (for Thursday) looks very ambitious. Let's treat that in a separated PR and keep this one for Links filtering 1. Please run npx nx run twenty-front:lint:ci, you should see some errors. (I believe they are tied to the changes you have made for sorting) 2. The work on Links looks good but I cannot test it as the front does not build at the moment 3. we'll have few lines to add in the backend to remove secondaryLinks from the FilterInput image.png (view on web) <https://github.com/twentyhq/twenty/assets/12035771/9b6f0909-8d3a-42d9-8f5d-58d696180fa6> — Reply to this email directly, view it on GitHub <#5787 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AU4NDQEI4NO5BFCQ5D2SX6LZGWGG3AVCNFSM6AAAAABJBAMXOSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJYGEYTKNRSGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

lucasbordeau

Thank you for contributing.

It's conceptually ok but some patterns feels odd, due to props drilling.

We use Recoil extensively in Twenty codebase and this feature particularly fits Recoil.

Could you please rework on a solution where :

Sorts are stored in a Recoil family state (you'll find many examples in our codebase) I suggest you make a family state with a key like { tableId: string, tableObjectName: string, tableFieldName: string } => value: OrderBy | null
The logic around a column header cell is abstracted in that header cell component and just modifies the related sort in the recoil family state
Then each list should just use a hook like useSortedArray that takes (arrayToSort: ObjectRecord[], tableId: string, tableObjectName: string and fieldName: string (or fieldName array string[]) and it applies the sort and just derives the state.

It will be much more simpler to read and maintain (and review ;) )

lucasbordeau · 2024-06-14T14:29:38Z

packages/twenty-front/src/hooks/useTableSort.ts

@@ -0,0 +1,62 @@
+import { useState } from 'react';
+
+enum sortOrders {


We already have OrderBy

lucasbordeau · 2024-06-14T14:31:01Z

packages/twenty-front/src/hooks/useTableSort.ts

+      : sortOrders.ascending;
+  };
+
+  const handleSortClick = (columnKey: keyof T) => {


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

lucasbordeau · 2024-06-14T14:32:24Z

packages/twenty-front/src/hooks/useTableSort.ts

+    sortConfig.sortOrder,
+  );
+
+  const toggleSortOrder = (sortOrder: sortOrders) => {


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

lucasbordeau · 2024-06-14T14:33:11Z

...rc/modules/settings/data-model/object-details/components/SettingsObjectFieldItemTableRow.tsx

@@ -17,11 +18,12 @@ import { SettingsObjectFieldDataType } from './SettingsObjectFieldDataType';

 type SettingsObjectFieldItemTableRowProps = {
  ActionIcon: ReactNode;
-  fieldMetadataItem: FieldMetadataItem;
+  fieldMetadataItem: FieldMetadataItem & { fieldType: string | boolean };


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

lucasbordeau · 2024-06-14T14:34:55Z

...rc/modules/settings/data-model/object-details/components/SettingsObjectFieldItemTableRow.tsx

@@ -60,6 +60,18 @@ export const SettingsObjectFieldItemTableRow = ({

  const fieldType = fieldMetadataItem.type;
  const isFieldTypeSupported = isFieldTypeSupportedInSettings(fieldType);
+  useEffect(() => {


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.

lucasbordeau · 2024-06-14T14:35:53Z

...rc/modules/settings/data-model/object-details/components/SettingsObjectFieldItemTableRow.tsx

-          !!identifierType &&
-          (identifierType === 'label' ? 'Record text' : 'Record image')}
-      </TableCell>
+      <TableCell>{fieldMetadataItem.fieldType}</TableCell>


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)

Add sort feature in settings page

e378ecf

greptile-apps bot reviewed Jun 9, 2024

View reviewed changes

Anand-Krishnan-M-J marked this pull request as draft June 9, 2024 16:46

FelixMalfait added the PR: awaiting review label Jun 10, 2024

lucasbordeau requested changes Jun 14, 2024

View reviewed changes

lucasbordeau self-assigned this Jun 14, 2024

lucasbordeau added PR: awaiting author and removed PR: awaiting review labels Jun 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Anand-Krishnan-M-J commented Jun 9, 2024

greptile-apps bot left a comment

greptile-apps bot Jun 9, 2024

greptile-apps bot Jun 9, 2024

greptile-apps bot Jun 9, 2024

greptile-apps bot Jun 9, 2024

greptile-apps bot Jun 9, 2024

greptile-apps bot Jun 9, 2024

greptile-apps bot Jun 9, 2024

greptile-apps bot Jun 9, 2024

Anand-Krishnan-M-J commented Jun 10, 2024 via email

lucasbordeau left a comment

lucasbordeau Jun 14, 2024

lucasbordeau Jun 14, 2024

lucasbordeau Jun 14, 2024

lucasbordeau Jun 14, 2024

lucasbordeau Jun 14, 2024

lucasbordeau Jun 14, 2024

		@@ -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';

		@@ -41,10 +43,8 @@ const StyledIconTableCell = styled(TableCell)`
		export const SettingsObjectFieldItemTableRow = ({

		@@ -1,7 +1,13 @@
		import { useEffect } from 'react';
		import { useCallback, useEffect, useState } from 'react';

		@@ -38,13 +64,74 @@ const StyledH1Title = styled(H1Title)`
		margin-bottom: 0;

		@@ -0,0 +1,62 @@
		import { useState } from 'react';

		enum sortOrders {

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

Are you sure you want to change the base?

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

Conversation

Anand-Krishnan-M-J commented Jun 9, 2024

Proposed Changes

Related Issue

Evidence

Further comments

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jun 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Jun 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Jun 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Jun 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Jun 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Jun 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Jun 9, 2024

Choose a reason for hiding this comment

greptile-apps bot Jun 9, 2024

Choose a reason for hiding this comment

Anand-Krishnan-M-J commented Jun 10, 2024 via email

lucasbordeau left a comment

Choose a reason for hiding this comment

lucasbordeau Jun 14, 2024

Choose a reason for hiding this comment

lucasbordeau Jun 14, 2024

Choose a reason for hiding this comment

lucasbordeau Jun 14, 2024

Choose a reason for hiding this comment

lucasbordeau Jun 14, 2024

Choose a reason for hiding this comment

lucasbordeau Jun 14, 2024

Choose a reason for hiding this comment

lucasbordeau Jun 14, 2024

Choose a reason for hiding this comment