feat: Subform table - enable editing of column title #14318
Conversation
github-actions
bot
added
area/ui-editor
lassopicasso
changed the title
lassopicasso
changed the title
lassopicasso
added
the
skip-releasenotes
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14318 +/- ##
=======================================
Coverage 95.66% 95.66%
=======================================
Files 1879 1880 +1
Lines 24405 24446 +41
Branches 2807 2810 +3
=======================================
+ Hits 23346 23386 +40
- Misses 799 800 +1
Partials 260 260 ☔ View full report in Codecov by Sentry. |
…github.com/Altinn/altinn-studio into subform-table-manually-editing-column-name
...itor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.ts
Outdated
Show resolved
Hide resolved
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the subform table columns configuration in the UX editor. The changes primarily involve updating language resources, modifying component props from Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx (1)
Line range hint
42-84: Add test cases for error scenariosThe test suite should include cases for:
- Failed text resource mutations
- Missing text resources
- Network errors
♻️ Duplicate comments (1)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.ts (1)
47-50: 🛠️ Refactor suggestionEnhance uniqueness of generated title IDs
As noted in a previous review, there's a risk of key collisions. Consider incorporating additional uniqueness factors.
export const getTitleIdForColumn = (titleId: string): string => { const prefixTitleId = 'subform_table_column_title_'; - return titleId.startsWith(prefixTitleId) ? titleId : prefixTitleId + getRandNumber(); + return titleId.startsWith(prefixTitleId) + ? titleId + : `${prefixTitleId}${Date.now()}_${getRandNumber()}`; };
🧹 Nitpick comments (7)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx (2)
26-33: Add aria-label to improve accessibilityThe StudioTextfield should have an aria-label for better screen reader support.
<StudioTextfield label={t('ux_editor.properties_panel.subform_table_columns.column_title_edit')} size='sm' value={title} autoFocus={true} + aria-label={t('ux_editor.properties_panel.subform_table_columns.column_title_edit')} onChange={(e) => setTitle(e.target.value)} error={!title?.trim() && errorMessage} />
30-32: Consider debouncing the onChange handlerFor better performance, consider debouncing the onChange handler to reduce the number of state updates when typing.
+import { debounce } from 'lodash'; + +const debouncedSetTitle = debounce((value: string) => setTitle(value), 300); + <StudioTextfield // ...other props - onChange={(e) => setTitle(e.target.value)} + onChange={(e) => debouncedSetTitle(e.target.value)} // ...other props />frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx (1)
Line range hint
41-48: Handle cleanup when unmounting during edit modeWhen the component unmounts while in edit mode, we should ensure proper cleanup to prevent any potential memory leaks or unexpected behavior.
+import { useEffect } from 'react'; +useEffect(() => { + return () => { + if (editing) { + onEdit(tableColumn); + } + }; +}, [editing, onEdit, tableColumn]);frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx (1)
Line range hint
64-71: Add error boundary for column renderingConsider wrapping the ColumnElement rendering in an error boundary to gracefully handle any rendering errors.
+import { ErrorBoundary } from 'react-error-boundary'; +const ColumnElementErrorFallback = ({ error, resetErrorBoundary }) => ( + <div> + {t('ux_editor.properties_panel.subform_table_columns.error_rendering_column')} + <button onClick={resetErrorBoundary}>Try again</button> + </div> +); {tableColumns.map((tableColumn: TableColumn, index: number) => ( + <ErrorBoundary + FallbackComponent={ColumnElementErrorFallback} + onReset={() => { + // Reset the error state + }} + > <ColumnElement subformLayout={component.layoutSet} key={getUniqueKey(index)} // ... other props /> + </ErrorBoundary> ))}frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.test.ts (2)
30-52: Consider extracting mock data to a separate fileThe
formLayoutsmock data structure is quite complex and could be reused across other test files. Consider moving it to a shared mock file to improve maintainability.
149-159: Add edge case test for getTitleIdForColumnWhile basic functionality is covered, consider adding a test case for empty string input to ensure robust error handling.
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx (1)
94-94: Add comment explaining the title format requirementThe assertion checks for 'subform_table_column_title_' prefix, but it's not immediately clear why this format is required. Consider adding a comment explaining the business rule.
+ // Column titles must follow the format 'subform_table_column_title_<number>' for proper text resource management expect(columnTitleKey).toEqual(expect.stringContaining('subform_table_column_title_'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/language/src/nb.json(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.test.tsx(3 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/ColumnElement.tsx(3 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx(2 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/EditSubformTableColumns.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.test.ts(3 hunks)frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElementContent.module.css
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (4)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.test.ts (1)
123-147: LGTM! Comprehensive test coverage for component filteringThe test cases effectively verify both successful filtering and empty result scenarios for
getComponentsForSubformTable.frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx (2)
79-80: LGTM! Good validation for save buttonThe save button disable logic effectively prevents invalid states.
46-48: Consider adding error handling for missing text resourcesThe
getValueOfTitleIdcall could potentially return undefined if text resources are not loaded. Consider adding a fallback value or loading state.frontend/language/src/nb.json (1)
1839-1843: LGTM! The translations are well-structured and consistent.The added Norwegian translations for subform table column configurations align well with the PR objectives and maintain consistency with existing translations.
Let's verify the consistency of these translations with other related keys:
✅ Verification successful
Translations are consistent and complete ✅
The new translations follow the established patterns in terms of:
- Key naming convention
- Norwegian terminology for columns ("kolonne")
- Label formatting and punctuation
- Error message style
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any inconsistencies in translation keys related to subform table columns # Look for related translation keys to ensure naming consistency rg -A 2 "subform_table_columns" frontend/language/src/Length of output: 2186
...itor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.ts
Outdated
Show resolved
Hide resolved
...nts/Properties/EditSubformTableColumns/ColumnElement/EditColumnElement/EditColumnElement.tsx
Show resolved
Hide resolved
...itor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.ts (3)
27-38: Enhance type safety and add JSDoc documentation.While the implementation is good, we can improve it further:
+/** + * Retrieves all form components that have both a title and a simple binding + * @param formLayouts - The form layouts to extract components from + * @returns Array of form items that match the criteria + */ export const getComponentsForSubformTable = (formLayouts: IFormLayouts): FormItem[] => { - const components = Object.values(formLayouts ?? {}).flatMap((layout: IInternalLayout) => + const components = Object.values(formLayouts ?? {}).flatMap((layout) => getAllLayoutComponents(layout), ); return componentsWithLabelAndDataModel(components); }; +/** + * Filters components that have both a title text resource binding and a simple data model binding + */ const componentsWithLabelAndDataModel = (components: FormItem[]): FormItem[] => {
40-42: Add error handling for missing text resources.The function should handle cases where the text resource or value is not found.
export const getValueOfTitleId = (titleId: string, textResources: ITextResources): string => { - return textResourceByLanguageAndIdSelector('nb', titleId)(textResources)?.value; + const value = textResourceByLanguageAndIdSelector('nb', titleId)(textResources)?.value; + if (!value) { + console.warn(`Text resource not found for titleId: ${titleId}`); + } + return value ?? ''; };
50-70: Add safety check for infinite loop scenario.While the implementation correctly uses subformId and ensures uniqueness, the while loop could potentially run indefinitely in an edge case where all possible IDs are taken.
export const getTitleIdForColumn = ({ titleId, subformId, textResources, }: TitleIdForColumn): string => { const prefixTitleId = 'subform_table_column_title_'; + const MAX_ATTEMPTS = 1000; // Prevent infinite loop + let attempts = 0; if (titleId.startsWith(prefixTitleId)) { return titleId; } const resourcesArray = Object.values(textResources).flat(); const isUnique = (id: string): boolean => !resourcesArray.some((resource) => resource.id === id); let uniqueTitleId = prefixTitleId + subformId; - while (!isUnique(uniqueTitleId)) { + while (!isUnique(uniqueTitleId) && attempts < MAX_ATTEMPTS) { uniqueTitleId = prefixTitleId + subformId + getRandNumber(); + attempts++; } + + if (attempts === MAX_ATTEMPTS) { + throw new Error('Failed to generate unique title ID after maximum attempts'); + } return uniqueTitleId; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (1)
frontend/packages/ux-editor/src/components/Properties/EditSubformTableColumns/utils/editSubformTableColumnsUtils.ts (1)
4-8: LGTM! Well-organized imports.The new imports are properly organized and typed, providing clear dependencies for the added functionality.
JamalAlabdullah
deleted the
subform-table-manually-editing-column-name
branch
Description
This PR enables users to write their custom title for the column in the subform table. This title will be a new text key solely used in the table form, so it won't influence the label of the component itself.
If the column is deleted, then this text key will also be deleted.
The cell content is now displayed as per the Figma design, and is not editable for now.
A simple validation has been added, where the save button is disabled until a component is selected. If the user removes the title completely, the save button will also be disabled.
subform-table-edit-title.mp4
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Improvements
Localization