feat: Config to remove edit and delete buttons texts in view mode and show them in the edit one. #3912
Conversation
JamalAlabdullah
added
the
backport-ignore
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
JamalAlabdullah
changed the title
|
@JamalAlabdullah The original issue is a feature request, so I would think this was meant to be an opt-in feature that could be toggled on the |
Ok understood, I will revert the changes, should i put it in blocked until we take another discussion ? |
|
I would just make it an optional feature. Dig around in the Just ping me on slack if anything is unclear, I can assist if needed! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/layout/RepeatingGroup/Table/RepeatingGroupTableRow.tsx (1)
299-305: Consider simplifying the nested ternary for readability.The logic is correct but the nested ternary operators reduce readability. Consider extracting to a variable or simplifying the expression.
🔎 Suggested simplification:
- {compactButtons - ? isEditingRow - ? deleteButtonText - : null - : isEditingRow || !mobileViewSmall - ? deleteButtonText - : null} + {(compactButtons ? isEditingRow : isEditingRow || !mobileViewSmall) + ? deleteButtonText + : null}Or extract to a variable before the JSX:
const showDeleteText = compactButtons ? isEditingRow : (isEditingRow || !mobileViewSmall); // ... {showDeleteText ? deleteButtonText : null}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/layout/RepeatingGroup/Table/RepeatingGroupTableRow.tsx(8 hunks)src/layout/RepeatingGroup/config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganytype or type casting (as type) in TypeScript code; improve typing by avoiding casts andanys when refactoring
Use objects for managing query keys and functions, andqueryOptionsfor sharing TanStack Query patterns across the system for central management
Files:
src/layout/RepeatingGroup/config.tssrc/layout/RepeatingGroup/Table/RepeatingGroupTableRow.tsx
{**/*.module.css,**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and leverage Digdir Design System components when possible
Files:
src/layout/RepeatingGroup/config.tssrc/layout/RepeatingGroup/Table/RepeatingGroupTableRow.tsx
src/layout/*/{config,Component,index,config.generated}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Layout components must follow standardized structure with
config.ts,Component.tsx,index.tsx, andconfig.generated.tsfiles
Files:
src/layout/RepeatingGroup/config.ts
🧬 Code graph analysis (1)
src/layout/RepeatingGroup/config.ts (1)
src/codegen/CG.ts (1)
CG(25-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
🔇 Additional comments (4)
src/layout/RepeatingGroup/config.ts (1)
217-226: LGTM! Configuration property is well-defined and follows established patterns.The
compactButtonsproperty is correctly placed under theeditconfig group as recommended in the PR discussion. The default value offalseensures this is an opt-in feature, avoiding breaking changes for existing service owners.src/layout/RepeatingGroup/Table/RepeatingGroupTableRow.tsx (3)
99-99: LGTM! Clean derivation of the configuration value.Using
Boolean()properly handles the optional property case, defaulting tofalsewhencompactButtonsis undefined.
343-367: LGTM! EditElement properly implements the compact buttons logic.The
showTextcalculation correctly handles both modes:
- When
compactButtonsis enabled: text appears only when the row is being edited (ariaExpanded)- When
compactButtonsis disabled: preserves original behavior (text shown when editing or on non-mobile views)
263-263: LGTM! Desktop delete button logic is clear and correct.When
compactButtonsis enabled, text is shown only during editing; otherwise, text is always displayed.
olemartinorg
changed the title
olemartinorg
added
kind/product-feature
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/layout/RepeatingGroup/Table/RepeatingGroupTable.test.tsx (3)
210-210: Prefer semantic selector over CSS class dependency.Using
classList.contains('tableButton')couples the test to implementation details (CSS class names). Consider using a test ID or more semantic selector like a combination of role and accessible name.🔎 View suggested refactor
Add a test ID to the table button in the component:
-const tableEditButton = editButtonsInEditMode.find((btn) => btn.classList.contains('tableButton')); +const tableEditButton = screen.getByTestId('repeating-group-table-edit-button-0');Alternatively, use a more specific query that doesn't rely on CSS classes:
-const tableEditButton = editButtonsInEditMode.find((btn) => btn.classList.contains('tableButton')); +// Find the button that's within the table row at index 0 +const tableEditButton = screen.getAllByRole('button', { name: /Lagre og lukk/i })[0];
213-215: Clarify implicit test assumptions about button ordering.The test assumes
deleteButtons[0]is in edit mode (has text) anddeleteButtons[1]is not (no text), but this isn't explicitly documented. Adding a comment would improve test readability.🔎 View suggested improvement
+// In edit mode: first row's delete button should show text, others remain icon-only const deleteButtons = screen.getAllByRole('button', { name: /Slett/i }); expect(deleteButtons[0]).toHaveTextContent('Slett'); expect(deleteButtons[1]).not.toHaveTextContent('Slett');
175-217: Consider adding test coverage for default behavior.The test suite validates
compactButtons: true, but doesn't verify the default behavior whencompactButtonsisfalseor undefined. Adding a test case for the default would ensure backwards compatibility and clarify expected behavior.🔎 View suggested test case
it('should show button text in both modes when compactButtons is false or not set', async () => { const groupWithoutCompactButtons = getFormLayoutRepeatingGroupMock({ id: 'mock-container-id', edit: { compactButtons: false }, }); const layout = getLayout(groupWithoutCompactButtons, components); await render(layout); // In view mode, buttons should have text on desktop const editButtons = screen.getAllByRole('button', { name: /Rediger/i }); const deleteButtons = screen.getAllByRole('button', { name: /Slett/i }); expect(editButtons[0]).toHaveTextContent('Rediger'); expect(deleteButtons[0]).toHaveTextContent('Slett'); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/layout/RepeatingGroup/Table/RepeatingGroupTable.test.tsx(2 hunks)src/layout/RepeatingGroup/Table/RepeatingGroupTableRow.tsx(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/layout/RepeatingGroup/Table/RepeatingGroupTableRow.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganytype or type casting (as type) in TypeScript code; improve typing by avoiding casts andanys when refactoring
Use objects for managing query keys and functions, andqueryOptionsfor sharing TanStack Query patterns across the system for central management
Files:
src/layout/RepeatingGroup/Table/RepeatingGroupTable.test.tsx
{**/*.module.css,**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and leverage Digdir Design System components when possible
Files:
src/layout/RepeatingGroup/Table/RepeatingGroupTable.test.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
renderWithProvidersfromsrc/test/renderWithProviders.tsxwhen testing components that require form layout context
Files:
src/layout/RepeatingGroup/Table/RepeatingGroupTable.test.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-25T12:53:54.399Z
Learnt from: CR
Repo: Altinn/app-frontend-react PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:53:54.399Z
Learning: Applies to **/*.test.{ts,tsx} : Use `renderWithProviders` from `src/test/renderWithProviders.tsx` when testing components that require form layout context
Applied to files:
src/layout/RepeatingGroup/Table/RepeatingGroupTable.test.tsx
🧬 Code graph analysis (1)
src/layout/RepeatingGroup/Table/RepeatingGroupTable.test.tsx (2)
src/test/mockMediaQuery.tsx (1)
mockMediaQuery(3-25)src/__mocks__/getFormLayoutGroupMock.ts (1)
getFormLayoutRepeatingGroupMock(4-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
🔇 Additional comments (2)
src/layout/RepeatingGroup/Table/RepeatingGroupTable.test.tsx (2)
175-179: LGTM: Test setup follows existing patterns.The test suite setup for
compactButtonscorrectly mirrors the existing desktop view test configuration.
235-246: LGTM: Text resources properly added.The new text resources correctly support the
compactButtonstest scenarios and maintain consistency with the existing Norwegian locale pattern.
|
JamalAlabdullah
added
the
squad/utforming
2 participants
Description
Changes:
Adds a new opt-in property compactButtons to the
RepeatingGroupcomponent'seditconfiguration. When enabled,edit = Redigeranddelete= Slettbuttons in the table view display only icons (without text labels) in view mode, while still showing full text labels when a row is in edit mode. This provides a cleaner, more compact table UI when there are many columns or limited horizontal space.How to Use
Add
compactButtons: true to theeditproperty of yourRepeatingGroupconfiguration in the layout JSON:SOLUTION:
Screen.Recording.2025-12-18.at.12.52.34.mov
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.