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

Conversation

Anand-Krishnan-M-J
Copy link
Contributor

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

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

Evidence after addressing review comments

2024-07-04.21-33-12.mp4

Further comments

Apologies for the large PR. Looking forward for the review

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

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

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.

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.

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

@@ -41,10 +43,8 @@ const StyledIconTableCell = styled(TableCell)`
export const SettingsObjectFieldItemTableRow = ({
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.

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,
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.

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@Anand-Krishnan-M-J
Copy link
Contributor Author

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

Copy link
Contributor

@lucasbordeau lucasbordeau left a comment

Choose a reason for hiding this comment

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

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 ;) )

@@ -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.

: 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

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()

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

type SettingsObjectFieldItemTableRowProps = {
ActionIcon: ReactNode;
fieldMetadataItem: FieldMetadataItem;
fieldMetadataItem: FieldMetadataItem & { fieldType: string | boolean };
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

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

const fieldType = fieldMetadataItem.type;
const isFieldTypeSupported = isFieldTypeSupportedInSettings(fieldType);
useEffect(() => {
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

!!identifierType &&
(identifierType === 'label' ? 'Record text' : 'Record image')}
</TableCell>
<TableCell>{fieldMetadataItem.fieldType}</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

@Anand-Krishnan-M-J
Copy link
Contributor Author

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 ;) )

Hi @lucasbordeau
I was wondering why we need to set the key as
{ tableId: string, tableObjectName: string, tableFieldName: string }

Can we keep the family state key simpler, like this?
{ tableId: string; initialFieldName: string }

**Example:**
export const tableSortFamilyState = createFamilyState<
  { fieldName: string; orderBy: OrderBy | null },
  { tableId: string; initialFieldName: string }
>({
  key: 'tableSortFamilyState',
  defaultValue: (({ initialFieldName }: { initialFieldName: string }) => ({
    fieldName: initialFieldName,
    orderBy: 'AscNullsLast',
  })) as unknown as {
    fieldName: string;
    orderBy: OrderBy | null;
  },
});

By using a key as an object comprising tableId and initialFieldName (since each sort is related to a table and has an initial default sort), the state value would be:

    {    fieldName: string;
         orderBy: OrderBy | null;
    }

Please let me know your thoughts on this

@Anand-Krishnan-M-J Anand-Krishnan-M-J marked this pull request as ready for review July 4, 2024 15:49
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Introduced useSortedArray hook for sorting table content (/packages/twenty-front/src/hooks/useSortedArray.ts)
  • Removed useTableSort hook and its test cases (/packages/twenty-front/src/hooks/useTableSort.ts, /packages/twenty-front/src/hooks/__tests__/useTableSort.test.ts)
  • Added Recoil state for managing table sort states (/packages/twenty-front/src/modules/activities/states/tabelSortFamilyState.ts)
  • Integrated sorting functionality into SettingsObjectFieldItemTableRow and SettingsObjectItemTableRow components (/packages/twenty-front/src/modules/settings/data-model/object-details/components/SettingsObjectFieldItemTableRow.tsx, /packages/twenty-front/src/modules/settings/data-model/object-details/components/SettingsObjectItemTableRow.tsx)
  • Updated SettingsObjectDetail and SettingsObjects pages to use new sorting feature (/packages/twenty-front/src/pages/settings/data-model/SettingsObjectDetail.tsx, /packages/twenty-front/src/pages/settings/data-model/SettingsObjects.tsx)

9 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings

packages/twenty-front/src/hooks/useSortedArray.ts Outdated Show resolved Hide resolved
TableSortFamilyKey
>({
key: 'tableSortFamilyState',
defaultValue: getDefaultTableSortState as unknown as TableSortState,
Copy link
Contributor

Choose a reason for hiding this comment

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

🪶 style: Casting getDefaultTableSortState as unknown and then TableSortState may hide potential type issues. Consider refining the type definitions.

Copy link
Member

Choose a reason for hiding this comment

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

It feels a bit odd, I'm not sure what you're trying to do here? Assigning a function as default value?

@Anand-Krishnan-M-J
Copy link
Contributor Author

Hey @lucasbordeau
Thanks for the detailed review and I have updated the PR.

I am still getting used to the Recoil pattern.
I come from more of a Redux - Saga pattern. Apologies for the trouble
Please let me know if there are still some weird things.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Added useSortedArray hook for sorting table content (/packages/twenty-front/src/hooks/useSortedArray.ts)
  • Introduced test cases for useSortedArray hook (/packages/twenty-front/src/hooks/__tests__/useSortedArray.test.tsx)
  • Corrected type casting in tableSortFamilyState.ts (/packages/twenty-front/src/modules/activities/states/tableSortFamilyState.ts)
  • Integrated sorting functionality into SettingsObjectDetail.tsx (/packages/twenty-front/src/pages/settings/data-model/SettingsObjectDetail.tsx)
  • Added sorting feature to SettingsObjects.tsx (/packages/twenty-front/src/pages/settings/data-model/SettingsObjects.tsx)

5 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

packages/twenty-front/src/hooks/useSortedArray.ts Outdated Show resolved Hide resolved
}: SettingsObjectItemTableRowProps) => {
const theme = useTheme();

const { totalCount } = useFindManyRecords({
objectNameSingular: objectItem.nameSingular,
});
useEffect(() => {
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

const SortIcon = ({ sortKey }: { sortKey: SortKeys }) => {
if (sortKey !== sortConfig.fieldName) return null;
return sortConfig.orderBy === 'AscNullsLast' ? (
<IconArrowUp size="14" />
Copy link
Member

Choose a reason for hiding this comment

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

We tried to avoid hardcoding size values like that, you can probably find examples in the code, getting the size from the theme directly. Also it feels like the arrow isn't perfectly positionned, it's a bit too closed to the name and not perfectly vertically aligned (a bit too high)

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

const tableHeadings: TableHeading = [
Copy link
Member

Choose a reason for hiding this comment

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

Something feels inconsistent here, it should be tableHeadings: TableHeading[] or tableHeading: TableHeading ; I think the singular version is best because heading is a row

Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like it should be TableFieldSortDefinition[] or something like that

@FelixMalfait
Copy link
Member

I think the order doesn't match with the arrows direction :)
Screenshot 2024-07-17 at 09 29 17
Screenshot 2024-07-17 at 09 29 41

@FelixMalfait
Copy link
Member

Otherwise really great PR! Thanks a lot for your work @Anand-Krishnan-M-J - sorry we took so long to review

instancesCount = 'instancesCount',
}
type InstanceCountStateType = { [key: string]: number };
type TableHeading = {
Copy link
Member

Choose a reason for hiding this comment

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

The developer experience you're creating for future sort use-cases isn't great, you have to redeclare types every time you want to add sort to a table. You're already duplicating some code between SettingsObjects and SettingsObjectDetail

packages/twenty-front/src/hooks/useSortedArray.ts Outdated Show resolved Hide resolved
packages/twenty-front/src/hooks/useSortedArray.ts Outdated Show resolved Hide resolved
order: OrderBy | null,
): T[] => {
return data.sort((a: T, b: T) => {
if (typeof a[columnKey] === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't try to guess the type from JS since it can be unreliable. Instead we should give this hook metadata about our table so that it knows how to behave for each field

packages/twenty-front/src/hooks/useSortedArray.ts Outdated Show resolved Hide resolved
sortKey: SortKeys.dataType,
},
];
const SortIcon = ({ sortKey }: { sortKey: SortKeys }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be move in the component that handles the column heading, it should be abstracted down.

@@ -29,7 +39,25 @@ import { Table } from '@/ui/layout/table/components/Table';
import { TableHeader } from '@/ui/layout/table/components/TableHeader';
import { TableSection } from '@/ui/layout/table/components/TableSection';
import { UndecoratedLink } from '@/ui/navigation/link/components/UndecoratedLink';

import { useSortedArray } from '~/hooks/useSortedArray';
export enum SortKeys {
Copy link
Contributor

Choose a reason for hiding this comment

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

SortKeys should be more self-explanatory, I cannot understand at first glance what it talks about.

Is it AvailableSortFieldName ?

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

const tableHeadings: TableHeading = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like it should be TableFieldSortDefinition[] or something like that

export const SettingsObjects = () => {
const [instanceCountObj, setInstanceCount] = useState<InstanceCountStateType>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming contains abbreviation and is not consistent

const theme = useTheme();

const { activeObjectMetadataItems, inactiveObjectMetadataItems } =
useFilteredObjectMetadataItems();
const { deleteOneObjectMetadataItem } = useDeleteOneObjectMetadataItem();
const { updateOneObjectMetadataItem } = useUpdateOneObjectMetadataItem();
const getRowData = (objectMetaDataItems: ObjectMetadataItem[]) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

getRowData is not self-explanatory enough

@lucasbordeau
Copy link
Contributor

@Anand-Krishnan-M-J Keep up the good work, I did a extensive code review so that you can be more in line with our standards and the way we code at Twenty.

@Anand-Krishnan-M-J
Copy link
Contributor Author

@lucasbordeau
Thank you for your detailed code review and for the encouragement. I will address all the points raised as soon as possible.
As I am currently managing a busy release schedule at work, I kindly ask you to bear with me for a little while longer.

@lucasbordeau
Copy link
Contributor

@Anand-Krishnan-M-J Please take your time, otherwise if it's too much work we'll handle it and update you on the changes for reference so that you can be up-to-date and ready for the next one ! Anyway it's already great to have made the core changes.

@Anand-Krishnan-M-J
Copy link
Contributor Author

@lucasbordeau
Thank you for understanding. Given the current time constraints and to avoid further delays in releasing this feature, please proceed with the updates. I will monitor the changes and learn from them to improve my future contributions.

@lucasbordeau lucasbordeau merged commit 59e14fa into twentyhq:main Aug 14, 2024
10 of 11 checks passed
Copy link

Thanks @Anand-Krishnan-M-J for your contribution!
This marks your 0th PR on the repo. You're top 0% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants