Aggregate follow-up #9547

ijreilly · 2025-01-10T16:25:20Z

In this PR

fix some UI regressions introduced by work on view groups
address some follow-ups:

Refactor around date aggregate operations

…olded

…ow-up-4

greptile-apps

PR Summary

This PR introduces significant changes to the aggregate operations system, particularly focusing on date-specific operations and UI improvements.

Added dedicated 'Dates' submenu in aggregate dropdowns for earliest/latest date operations via new DATE_AGGREGATE_OPERATIONS enum
Replaced AGGREGATE_OPERATIONS with ExtendedAggregateOperations type across components for better type safety
Fixed sticky positioning and styling for aggregate footer in tables with proper shadow effects
Added visual selection indicator for 'None' option in aggregate menus
Improved dropdown header styling to maintain selected state when menu is open

_{45 file(s) reviewed, 22 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-01-10T16:26:20Z

...-front/src/modules/object-record/object-filter-dropdown/components/MultipleFiltersButton.tsx

    toggleDropdown();
    resetFilterDropdown();


style: toggling dropdown before resetting could cause visual glitches - consider resetting first

greptile-apps · 2025-01-10T16:27:02Z

...ord-board/record-board-column/components/RecordBoardColumnHeaderAggregateDropdownContent.tsx

+          [
+            DATE_AGGREGATE_OPERATIONS.earliest,
+            DATE_AGGREGATE_OPERATIONS.latest,
+          ],


style: Consider using DATE_AGGREGATE_OPERATIONS as an array directly rather than hardcoding array elements to avoid potential sync issues if new operations are added

greptile-apps · 2025-01-10T16:29:39Z

...odules/object-record/record-board/record-board-column/utils/computeAggregateValueAndLabel.ts

+  if (
+    COUNT_AGGREGATE_OPERATION_OPTIONS.includes(
+      aggregateOperation as AGGREGATE_OPERATIONS,
+    )
+  ) {


logic: type assertion here could mask potential type mismatches between ExtendedAggregateOperations and AGGREGATE_OPERATIONS

greptile-apps · 2025-01-10T16:30:56Z

...ages/twenty-front/src/modules/object-record/record-index/components/RecordIndexContainer.tsx

+          const convertedViewFieldAggregateOperation = isDefined(
+            viewField.aggregateOperation,
+          )
+            ? convertAggregateOperationToExtendedAggregateOperation(
+                viewField.aggregateOperation,
+                viewFieldMetadataType,
+              )
+            : viewField.aggregateOperation;
+


logic: convertAggregateOperationToExtendedAggregateOperation is called even when viewFieldMetadataType is undefined, which could cause runtime errors

greptile-apps · 2025-01-10T16:35:26Z

...ges/twenty-front/src/modules/object-record/record-table/constants/DateAggregateOperations.ts

+  earliest = 'EARLIEST',
+  latest = 'LATEST',


style: consider using lowercase enum values ('earliest' instead of 'EARLIEST') to match TypeScript conventions, since these are string enums

greptile-apps · 2025-01-10T16:43:51Z

...front/src/modules/object-record/utils/getAvailableFieldsIdsForAggregationFromObjectFields.ts

    if (isDefined(allAggregations[field.name])) {
      Object.keys(allAggregations[field.name]).forEach((aggregation) => {


style: Accessing allAggregations[field.name] twice - once for isDefined check and once for Object.keys. Consider storing in a variable.

greptile-apps · 2025-01-10T16:44:28Z

...ont/src/modules/object-record/utils/convertAggregateOperationToExtendedAggregateOperation.ts

@@ -9,10 +10,10 @@ export const convertAggregateOperationToExtendedAggregateOperation = (
 ): ExtendedAggregateOperations => {
  if (isFieldMetadataDateKind(fieldType) === true) {


style: explicit comparison with true is redundant, could be simplified to just if (isFieldMetadataDateKind(fieldType))

greptile-apps · 2025-01-10T16:44:29Z

...ont/src/modules/object-record/utils/convertAggregateOperationToExtendedAggregateOperation.ts

    if (aggregateOperation === AGGREGATE_OPERATIONS.min) {
-      return 'EARLIEST';
+      return DATE_AGGREGATE_OPERATIONS.earliest;
    }
    if (aggregateOperation === AGGREGATE_OPERATIONS.max) {
-      return 'LATEST';
+      return DATE_AGGREGATE_OPERATIONS.latest;
    }


style: consider using a switch statement for better readability and maintainability when handling multiple cases

greptile-apps · 2025-01-10T16:46:19Z

packages/twenty-front/src/modules/ui/layout/dropdown/components/Dropdown.tsx

+type StyledHeaderDivProps = {
+  isUnfolded?: boolean;
+  isActive?: boolean;
+};


style: isActive prop is defined but never used in the component

greptile-apps · 2025-01-10T16:46:20Z

packages/twenty-front/src/modules/ui/layout/dropdown/components/Dropdown.tsx

@@ -1,37 +1,56 @@
+import { DropdownContent } from '@/ui/layout/dropdown/components/DropdownContent';
+import { DropdownOnToggleEffect } from '@/ui/layout/dropdown/components/DropdownOnToggleEffect';
+import { DropdownComponentInstanceContext } from '@/ui/layout/dropdown/contexts/DropdownComponeInstanceContext';


syntax: 'DropdownComponeInstanceContext' contains a typo in the import path ('Compone' should be 'Component')

Weiko

LGTM! Good job 👏

Weiko · 2025-01-10T17:47:52Z

...ages/twenty-front/src/modules/object-record/record-index/components/RecordIndexContainer.tsx

@@ -125,6 +127,9 @@ export const RecordIndexContainer = () => {
        }


Seems there is duplicated logic between packages/twenty-front/src/modules/object-record/record-index/components/RecordIndexTableContainerEffect.tsx and packages/twenty-front/src/modules/object-record/record-index/components/RecordIndexContainer.tsx
Should we create a shared hook for that?

taking a note of that. I may refactor this part again as we discussed (in a later PR), to handle the conversions in the back-end, which would make that logic disappear

ijreilly added 9 commits January 9, 2025 17:39

Move dates to a different category of aggregate operation

531d4fe

Always persist background color on dropdown clickable button when unf…

1aa4c2b

…olded

more css fixes

2b3633b

Merge branch 'main' of github.com:twentyhq/twenty into aggregate-foll…

83bdc06

…ow-up-4

Fix regressions on aggregate footer bars

2ddfbe5

Add none option at first menu of aggregate dropdown

cff6b16

Refactor dates aggregate operations

f98aefe

Merge branch 'main' of github.com:twentyhq/twenty into aggregate-foll…

adfb7ac

…ow-up-4

fix dates

226b59a

greptile-apps bot reviewed Jan 10, 2025

View reviewed changes

ijreilly added 2 commits January 10, 2025 17:55

Fix dropdown button should keep selected when open + feature flag

f308ad0

Add border-top to aggregate bar

3f2f4bb

Weiko approved these changes Jan 10, 2025

View reviewed changes

fix dropdown button color

f6030ec

ijreilly enabled auto-merge (squash) January 10, 2025 18:48

charlesBochet disabled auto-merge January 10, 2025 19:01

charlesBochet merged commit 2e0169b into main Jan 10, 2025
31 of 33 checks passed

charlesBochet deleted the aggregate-follow-up-4 branch January 10, 2025 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregate follow-up #9547

Aggregate follow-up #9547

ijreilly commented Jan 10, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Jan 10, 2025

greptile-apps bot Jan 10, 2025

greptile-apps bot Jan 10, 2025

greptile-apps bot Jan 10, 2025

greptile-apps bot Jan 10, 2025

greptile-apps bot Jan 10, 2025

greptile-apps bot Jan 10, 2025

greptile-apps bot Jan 10, 2025

greptile-apps bot Jan 10, 2025

greptile-apps bot Jan 10, 2025

Weiko left a comment •

edited

Loading

Weiko Jan 10, 2025

ijreilly Jan 10, 2025

		if (isDefined(allAggregations[field.name])) {
		Object.keys(allAggregations[field.name]).forEach((aggregation) => {

		@@ -9,10 +10,10 @@ export const convertAggregateOperationToExtendedAggregateOperation = (
		): ExtendedAggregateOperations => {
		if (isFieldMetadataDateKind(fieldType) === true) {

		@@ -125,6 +127,9 @@ export const RecordIndexContainer = () => {
		}

Aggregate follow-up #9547

Aggregate follow-up #9547

Conversation

ijreilly commented Jan 10, 2025 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jan 10, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 10, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 10, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 10, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 10, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 10, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 10, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 10, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 10, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 10, 2025

Choose a reason for hiding this comment

Weiko left a comment • edited Loading

Choose a reason for hiding this comment

Weiko Jan 10, 2025

Choose a reason for hiding this comment

ijreilly Jan 10, 2025

Choose a reason for hiding this comment

ijreilly commented Jan 10, 2025 •

edited

Loading

Weiko left a comment •

edited

Loading