From ab0cb676863358f366126219980b6e97e9d472ec Mon Sep 17 00:00:00 2001 From: nitin <142569587+ehconitin@users.noreply.github.com> Date: Thu, 30 Oct 2025 20:56:37 +0530 Subject: [PATCH] Render depends on menu items conditionally in chart settings + fix min max values (#15391) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes https://discord.com/channels/1130383047699738754/1430894689317294152 --------- Co-authored-by: Lucas Bordeau Co-authored-by: Raphaël Bosi <71827178+bosiraphael@users.noreply.github.com> --- .../components/CommandMenuItemNumberInput.tsx | 5 +- .../page-layout/components/ChartSettings.tsx | 102 ++++++++++-------- .../chart-settings/ChartSettingItem.tsx | 13 +-- .../__tests__/isMinMaxRangeValid.test.ts | 96 +++++++++++++++++ ...test.ts => shouldHideChartSetting.test.ts} | 26 ++--- .../page-layout/utils/isMinMaxRangeValid.ts | 6 +- ...gDisabled.ts => shouldHideChartSetting.ts} | 2 +- .../menu-item/components/MenuItemToggle.tsx | 12 ++- 8 files changed, 185 insertions(+), 77 deletions(-) rename packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/__tests__/{isChartSettingDisabled.test.ts => shouldHideChartSetting.test.ts} (87%) rename packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/{isChartSettingDisabled.ts => shouldHideChartSetting.ts} (95%) diff --git a/packages/twenty-front/src/modules/command-menu/components/CommandMenuItemNumberInput.tsx b/packages/twenty-front/src/modules/command-menu/components/CommandMenuItemNumberInput.tsx index 84536220be3..bd395a612c4 100644 --- a/packages/twenty-front/src/modules/command-menu/components/CommandMenuItemNumberInput.tsx +++ b/packages/twenty-front/src/modules/command-menu/components/CommandMenuItemNumberInput.tsx @@ -49,8 +49,8 @@ export const CommandMenuItemNumberInput = ({ const numericValue = castAsNumberOrNull(draftValue); if (isDefined(onValidate)) { - const isInvalid = onValidate(numericValue); - if (isInvalid) { + const isValid = onValidate(numericValue); + if (!isValid) { setHasError(true); return; } @@ -86,6 +86,7 @@ export const CommandMenuItemNumberInput = ({ onBlur={handleBlur} onKeyDown={handleKeyDown} placeholder={placeholder} + error={hasError ? ' ' : undefined} noErrorHelper /> ); diff --git a/packages/twenty-front/src/modules/command-menu/pages/page-layout/components/ChartSettings.tsx b/packages/twenty-front/src/modules/command-menu/pages/page-layout/components/ChartSettings.tsx index 917371933b7..eccb1e8b78c 100644 --- a/packages/twenty-front/src/modules/command-menu/pages/page-layout/components/ChartSettings.tsx +++ b/packages/twenty-front/src/modules/command-menu/pages/page-layout/components/ChartSettings.tsx @@ -14,7 +14,7 @@ import { useUpdateChartSettingToggle } from '@/command-menu/pages/page-layout/ho import { useUpdateCurrentWidgetConfig } from '@/command-menu/pages/page-layout/hooks/useUpdateCurrentWidgetConfig'; import { type ChartConfiguration } from '@/command-menu/pages/page-layout/types/ChartConfiguration'; import { CHART_CONFIGURATION_SETTING_IDS } from '@/command-menu/pages/page-layout/types/ChartConfigurationSettingIds'; -import { isChartSettingDisabled } from '@/command-menu/pages/page-layout/utils/isChartSettingDisabled'; +import { shouldHideChartSetting } from '@/command-menu/pages/page-layout/utils/shouldHideChartSetting'; import { CommandMenuPages } from '@/command-menu/types/CommandMenuPages'; import { useObjectMetadataItems } from '@/object-metadata/hooks/useObjectMetadataItems'; import { GRAPH_MAXIMUM_NUMBER_OF_GROUPS } from '@/page-layout/widgets/graph/constants/GraphMaximumNumberOfGroups.constant'; @@ -123,13 +123,21 @@ export const ChartSettings = ({ widget }: { widget: PageLayoutWidget }) => { const chartSettings = GRAPH_TYPE_INFORMATION[currentGraphType].settings; + const visibleItemIds = chartSettings.flatMap((group) => + group.items + .filter( + (item) => + !shouldHideChartSetting( + item, + widget.objectMetadataId, + isGroupByEnabled as boolean, + ), + ) + .map((item) => item.id), + ); + return ( - group.items.map((item) => item.id)), - ]} - > + { message={t`Max ${GRAPH_MAXIMUM_NUMBER_OF_GROUPS} bars per chart. Consider adding a filter`} /> )} - {chartSettings.map((group) => ( - - {group.items.map((item) => { - const isDisabled = isChartSettingDisabled( + {chartSettings.map((group) => { + const visibleItems = group.items.filter( + (item) => + !shouldHideChartSetting( item, widget.objectMetadataId, isGroupByEnabled as boolean, - ); + ), + ); - const handleItemToggleChange = () => { - setSelectedItemId(item.id); - updateChartSettingToggle(item.id); - }; + return ( + + {visibleItems.map((item) => { + const handleItemToggleChange = () => { + setSelectedItemId(item.id); + updateChartSettingToggle(item.id); + }; - const handleItemInputChange = (value: number | null) => { - updateChartSettingInput(item.id, value); - }; + const handleItemInputChange = (value: number | null) => { + updateChartSettingInput(item.id, value); + }; - const handleItemDropdownOpen = () => { - openDropdown({ - dropdownComponentInstanceIdFromProps: item.id, - }); - }; + const handleItemDropdownOpen = () => { + openDropdown({ + dropdownComponentInstanceIdFromProps: item.id, + }); + }; - const handleFilterClick = () => { - navigatePageLayoutCommandMenu({ - commandMenuPage: CommandMenuPages.PageLayoutGraphFilter, - }); - }; + const handleFilterClick = () => { + navigatePageLayoutCommandMenu({ + commandMenuPage: CommandMenuPages.PageLayoutGraphFilter, + }); + }; - return ( - - ); - })} - - ))} + return ( + + ); + })} + + ); + })} ); }; diff --git a/packages/twenty-front/src/modules/command-menu/pages/page-layout/components/chart-settings/ChartSettingItem.tsx b/packages/twenty-front/src/modules/command-menu/pages/page-layout/components/chart-settings/ChartSettingItem.tsx index 68eadc0b647..8f8e795cd7d 100644 --- a/packages/twenty-front/src/modules/command-menu/pages/page-layout/components/chart-settings/ChartSettingItem.tsx +++ b/packages/twenty-front/src/modules/command-menu/pages/page-layout/components/chart-settings/ChartSettingItem.tsx @@ -14,7 +14,6 @@ import { isDefined } from 'twenty-shared/utils'; type ChartSettingItemProps = { item: ChartSettingsItem; - isDisabled: boolean; configuration: ChartConfiguration; getChartSettingsValues: ( itemId: CHART_CONFIGURATION_SETTING_IDS, @@ -27,7 +26,6 @@ type ChartSettingItemProps = { export const ChartSettingItem = ({ item, - isDisabled, configuration, getChartSettingsValues, onToggleChange, @@ -68,7 +66,7 @@ export const ChartSettingItem = ({ value={stringValue} onChange={onInputChange} onValidate={(value) => - isDefined(value) && + !isDefined(value) || isMinMaxRangeValid( item.id as | CHART_CONFIGURATION_SETTING_IDS.MIN_RANGE @@ -92,7 +90,7 @@ export const ChartSettingItem = ({ + ); diff --git a/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/__tests__/isMinMaxRangeValid.test.ts b/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/__tests__/isMinMaxRangeValid.test.ts index d47b9dba4b6..bf1afb90de8 100644 --- a/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/__tests__/isMinMaxRangeValid.test.ts +++ b/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/__tests__/isMinMaxRangeValid.test.ts @@ -114,4 +114,100 @@ describe('isMinMaxRangeValid', () => { expect(result).toBe(true); }); }); + + describe('setting first value (empty configuration)', () => { + it('should be valid when setting rangeMin on configuration without any range values', () => { + const emptyConfig = { + __typename: 'BarChartConfiguration', + graphType: GraphType.VERTICAL_BAR, + } as ChartConfiguration; + + const result = isMinMaxRangeValid( + CHART_CONFIGURATION_SETTING_IDS.MIN_RANGE, + 50, + emptyConfig, + ); + + expect(result).toBe(true); + }); + + it('should be valid when setting rangeMax on configuration without any range values', () => { + const emptyConfig = { + __typename: 'BarChartConfiguration', + graphType: GraphType.VERTICAL_BAR, + } as ChartConfiguration; + + const result = isMinMaxRangeValid( + CHART_CONFIGURATION_SETTING_IDS.MAX_RANGE, + 100, + emptyConfig, + ); + + expect(result).toBe(true); + }); + + it('should be valid when setting rangeMin without existing rangeMax', () => { + const configWithOnlyMin = { + __typename: 'BarChartConfiguration', + graphType: GraphType.VERTICAL_BAR, + rangeMin: 10, + } as ChartConfiguration; + + const result = isMinMaxRangeValid( + CHART_CONFIGURATION_SETTING_IDS.MIN_RANGE, + 20, + configWithOnlyMin, + ); + + expect(result).toBe(true); + }); + + it('should be valid when setting rangeMax without existing rangeMin', () => { + const configWithOnlyMax = { + __typename: 'BarChartConfiguration', + graphType: GraphType.VERTICAL_BAR, + rangeMax: 100, + } as ChartConfiguration; + + const result = isMinMaxRangeValid( + CHART_CONFIGURATION_SETTING_IDS.MAX_RANGE, + 150, + configWithOnlyMax, + ); + + expect(result).toBe(true); + }); + + it('should be invalid when setting rangeMin that would exceed existing rangeMax', () => { + const configWithOnlyMax = { + __typename: 'BarChartConfiguration', + graphType: GraphType.VERTICAL_BAR, + rangeMax: 100, + } as ChartConfiguration; + + const result = isMinMaxRangeValid( + CHART_CONFIGURATION_SETTING_IDS.MIN_RANGE, + 150, + configWithOnlyMax, + ); + + expect(result).toBe(false); + }); + + it('should be invalid when setting rangeMax that would be less than existing rangeMin', () => { + const configWithOnlyMin = { + __typename: 'BarChartConfiguration', + graphType: GraphType.VERTICAL_BAR, + rangeMin: 50, + } as ChartConfiguration; + + const result = isMinMaxRangeValid( + CHART_CONFIGURATION_SETTING_IDS.MAX_RANGE, + 10, + configWithOnlyMin, + ); + + expect(result).toBe(false); + }); + }); }); diff --git a/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/__tests__/isChartSettingDisabled.test.ts b/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/__tests__/shouldHideChartSetting.test.ts similarity index 87% rename from packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/__tests__/isChartSettingDisabled.test.ts rename to packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/__tests__/shouldHideChartSetting.test.ts index 0edba268cb3..18129cf2391 100644 --- a/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/__tests__/isChartSettingDisabled.test.ts +++ b/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/__tests__/shouldHideChartSetting.test.ts @@ -2,9 +2,9 @@ import { CHART_CONFIGURATION_SETTING_IDS } from '@/command-menu/pages/page-layou import { type ChartSettingsItem } from '@/command-menu/pages/page-layout/types/ChartSettingsGroup'; import { msg } from '@lingui/core/macro'; import { IconChartBar } from 'twenty-ui/display'; -import { isChartSettingDisabled } from '../isChartSettingDisabled'; +import { shouldHideChartSetting } from '../shouldHideChartSetting'; -describe('isChartSettingDisabled', () => { +describe('shouldHideChartSetting', () => { const mockItemWithoutDependencies: ChartSettingsItem = { id: CHART_CONFIGURATION_SETTING_IDS.DATA_LABELS, label: msg`Data Labels`, @@ -45,7 +45,7 @@ describe('isChartSettingDisabled', () => { describe('item without dependencies', () => { it('should return false when item has no dependencies', () => { - const result = isChartSettingDisabled( + const result = shouldHideChartSetting( mockItemWithoutDependencies, 'valid-object-id', true, @@ -55,7 +55,7 @@ describe('isChartSettingDisabled', () => { }); it('should return false when item has no dependencies even with no object metadata', () => { - const result = isChartSettingDisabled( + const result = shouldHideChartSetting( mockItemWithoutDependencies, '', false, @@ -67,7 +67,7 @@ describe('isChartSettingDisabled', () => { describe('item depending on SOURCE', () => { it('should return true when no object metadata and item depends on SOURCE', () => { - const result = isChartSettingDisabled( + const result = shouldHideChartSetting( mockItemDependingOnSource, '', true, @@ -77,7 +77,7 @@ describe('isChartSettingDisabled', () => { }); it('should return false when object metadata exists and item depends on SOURCE', () => { - const result = isChartSettingDisabled( + const result = shouldHideChartSetting( mockItemDependingOnSource, 'valid-object-id', true, @@ -89,7 +89,7 @@ describe('isChartSettingDisabled', () => { describe('item depending on GROUP_BY', () => { it('should return true when group by is not enabled and item depends on GROUP_BY', () => { - const result = isChartSettingDisabled( + const result = shouldHideChartSetting( mockItemDependingOnGroupBy, 'valid-object-id', false, @@ -99,7 +99,7 @@ describe('isChartSettingDisabled', () => { }); it('should return false when group by is enabled and item depends on GROUP_BY', () => { - const result = isChartSettingDisabled( + const result = shouldHideChartSetting( mockItemDependingOnGroupBy, 'valid-object-id', true, @@ -111,7 +111,7 @@ describe('isChartSettingDisabled', () => { describe('item with multiple dependencies', () => { it('should return true if any dependency is not met (no object metadata)', () => { - const result = isChartSettingDisabled( + const result = shouldHideChartSetting( mockItemWithMultipleDependencies, '', true, @@ -121,7 +121,7 @@ describe('isChartSettingDisabled', () => { }); it('should return true if any dependency is not met (group by disabled)', () => { - const result = isChartSettingDisabled( + const result = shouldHideChartSetting( mockItemWithMultipleDependencies, 'valid-object-id', false, @@ -131,7 +131,7 @@ describe('isChartSettingDisabled', () => { }); it('should return false if all dependencies are met', () => { - const result = isChartSettingDisabled( + const result = shouldHideChartSetting( mockItemWithMultipleDependencies, 'valid-object-id', true, @@ -152,7 +152,7 @@ describe('isChartSettingDisabled', () => { dependsOn: undefined, }; - const result = isChartSettingDisabled( + const result = shouldHideChartSetting( itemWithUndefinedDependsOn, '', false, @@ -171,7 +171,7 @@ describe('isChartSettingDisabled', () => { dependsOn: [], }; - const result = isChartSettingDisabled(itemWithEmptyDependsOn, '', false); + const result = shouldHideChartSetting(itemWithEmptyDependsOn, '', false); expect(result).toBe(false); }); diff --git a/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/isMinMaxRangeValid.ts b/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/isMinMaxRangeValid.ts index 4434dd2bf0f..3c3961f8cca 100644 --- a/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/isMinMaxRangeValid.ts +++ b/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/isMinMaxRangeValid.ts @@ -9,12 +9,9 @@ export const isMinMaxRangeValid = ( newValue: number, configuration: ChartConfiguration, ): boolean => { - if (!('rangeMax' in configuration || 'rangeMin' in configuration)) { - return false; - } - if (settingId === CHART_CONFIGURATION_SETTING_IDS.MIN_RANGE) { if ( + 'rangeMax' in configuration && isDefined(configuration.rangeMax) && newValue > configuration.rangeMax ) { @@ -24,6 +21,7 @@ export const isMinMaxRangeValid = ( if (settingId === CHART_CONFIGURATION_SETTING_IDS.MAX_RANGE) { if ( + 'rangeMin' in configuration && isDefined(configuration.rangeMin) && newValue < configuration.rangeMin ) { diff --git a/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/isChartSettingDisabled.ts b/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/shouldHideChartSetting.ts similarity index 95% rename from packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/isChartSettingDisabled.ts rename to packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/shouldHideChartSetting.ts index 8df0820de6c..c13e4d3f2f8 100644 --- a/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/isChartSettingDisabled.ts +++ b/packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/shouldHideChartSetting.ts @@ -2,7 +2,7 @@ import { CHART_CONFIGURATION_SETTING_IDS } from '@/command-menu/pages/page-layou import { type ChartSettingsItem } from '@/command-menu/pages/page-layout/types/ChartSettingsGroup'; import { isNonEmptyString } from '@sniptt/guards'; -export const isChartSettingDisabled = ( +export const shouldHideChartSetting = ( item: ChartSettingsItem, objectMetadataId: string, isGroupByEnabled: boolean, diff --git a/packages/twenty-ui/src/navigation/menu/menu-item/components/MenuItemToggle.tsx b/packages/twenty-ui/src/navigation/menu/menu-item/components/MenuItemToggle.tsx index 61ad8fb29c6..8d1cc9a769b 100644 --- a/packages/twenty-ui/src/navigation/menu/menu-item/components/MenuItemToggle.tsx +++ b/packages/twenty-ui/src/navigation/menu/menu-item/components/MenuItemToggle.tsx @@ -25,6 +25,7 @@ export type MenuItemToggleProps = { className?: string; onToggleChange?: (toggled: boolean) => void; toggleSize?: ToggleSize; + disabled?: boolean; }; export const MenuItemToggle = ({ @@ -36,22 +37,29 @@ export const MenuItemToggle = ({ className, onToggleChange, toggleSize, + disabled = false, }: MenuItemToggleProps) => { const instanceId = useId(); return ( - +