Improve aggregate footer cell display #9124
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR improves aggregate value display in table footers and fixes related rendering issues across the application.
- Replaced falsy checks with
isDefined()inobject-records-to-graphql-connection.helper.tsto properly handle zero values in aggregations - Added specialized formatting for different field types (percentage, decimals) in
computeAggregateValueAndLabel.ts - Added new
aggregateFooterCellscroll wrapper context inScrollWrapperContexts.tsxfor better footer cell behavior - Hardcoded UUID for 'No Value' column in
mapViewGroupsToRecordGroupDefinitions.tsto prevent unnecessary re-renders - Standardized first column width using
FIRST_TH_WIDTHconstant across table components for consistency
14 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
| text={value ? formatNumber(Number(value)) : '-'} | ||
| color={'transparent'} | ||
| /> | ||
| <Tag text={value ? value.toString() : '-'} color={'transparent'} /> |
There was a problem hiding this comment.
logic: removing formatNumber() means numbers won't be properly formatted (e.g. 1000000 will show as '1000000' instead of '1M' or '1,000,000'). Consider keeping number formatting for better readability
| } else if (!isDefined(aggregateValue)) { | ||
| value = '-'; |
There was a problem hiding this comment.
style: consider using null instead of '-' to maintain consistent typing across the codebase
| {isDefined(aggregateValue) ? ( | ||
| <StyledValueContainer> | ||
| <StyledLabel>{aggregateLabel}</StyledLabel> | ||
| <StyledValue>{aggregateValue}</StyledValue> | ||
| </StyledValueContainer> |
There was a problem hiding this comment.
style: StyledValueContainer's flex-grow behavior combined with flex: 1 0 0 might cause layout issues in narrow columns. Consider using flex: 1 1 auto instead to allow shrinking.
| <ScrollWrapper | ||
| contextProviderName="aggregateFooterCell" | ||
| componentInstanceId="AggregateFooterCellScrollWrapperContext" | ||
| defaultEnableYScroll={false} | ||
| > |
There was a problem hiding this comment.
logic: Using the same componentInstanceId 'AggregateFooterCellScrollWrapperContext' for all footer cells could cause conflicts if multiple cells need independent scroll contexts
| @@ -4,14 +4,15 @@ import { MOBILE_VIEWPORT } from 'twenty-ui'; | |||
| import { TABLE_CELL_CHECKBOX_MIN_WIDTH } from '@/object-record/record-table/record-table-cell/components/RecordTableCellCheckbox'; | |||
| import { TABLE_CELL_GRIP_WIDTH } from '@/object-record/record-table/record-table-cell/components/RecordTableCellGrip'; | |||
| import { RecordTableFooterCell } from '@/object-record/record-table/record-table-footer/components/RecordTableFooterCell'; | |||
| import { FIRST_TH_WIDTH } from '@/object-record/record-table/record-table-header/components/RecordTableHeader'; | |||
| import { visibleTableColumnsComponentSelector } from '@/object-record/record-table/states/selectors/visibleTableColumnsComponentSelector'; | |||
| import { useRecoilComponentValueV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentValueV2'; | |||
|
|
|||
| const StyledTableFoot = styled.thead` | |||
There was a problem hiding this comment.
logic: thead element is used for footer markup which is semantically incorrect
No description provided.