chore: implement addbutton for optional fileds #17608
Conversation
github-actions
bot
added
skip-releasenotes
📝 WalkthroughWalkthroughAdds a new StudioInlineTextField component with view/edit inline behaviour, integrates it into AppConfigForm for homepage and keywords, updates tests and public exports, adds CSS, and adjusts Norwegian localisation keys and text. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (tester)
participant App as AppConfigForm
participant Inline as StudioInlineTextField
participant TestH as Test Helper
User->>App: open About tab
App->>Inline: render view mode (label + value)
User->>Inline: click label/button (enter edit)
Inline->>Inline: set isEditMode=true, populate inputValue
TestH->>Inline: find inline textbox (getOptionalInlineEditTextbox)
TestH->>Inline: type new value
TestH->>Inline: click save (getInlineEditSaveButton)
Inline->>App: onChange(newValue)
Inline->>Inline: set isEditMode=false
App->>App: update local appConfig state
User->>App: click form save
App->>App: persist updated config (form submit)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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 |
![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
🤖 Fix all issues with AI agents
In `@src/Designer/frontend/language/src/nb.json`:
- Around line 302-305: The value for the localization key
"app_settings.about_tab_homepage_field_description" contains a stray "n"; update
that JSON string to read "Lim inn lenke til siden der sluttbrukere finner appen
og informasjon om den." (i.e., remove the extra "n" after "lenke" and ensure
proper spacing/punctuation), leaving the other keys
("app_settings.about_tab_homepage_field_label",
"app_settings.about_tab_keywords_description",
"app_settings.about_tab_keywords_label") unchanged.
In
`@src/Designer/frontend/libs/studio-components/src/components/StudioInlineEdit/StudioInlineEdit.tsx`:
- Around line 11-20: The icon-only action buttons in StudioInlineEdit lack
accessible names; update StudioInlineEditProps to accept caller-provided
aria-label strings (e.g., saveAriaLabel and cancelAriaLabel, and editAriaLabel
if there is an edit action) and use those props when rendering the icon buttons
inside the StudioInlineEdit component (replace the current icon-only
Button/IconButton usages around the save/cancel render paths referenced in the
component). Ensure the component falls back to sensible localised defaults when
props are not provided and apply the aria-label attributes to the exact button
elements that currently render only icons (the save and cancel action buttons
around lines ~83-88 and the edit/action buttons at ~22-31).
🧹 Nitpick comments (1)
src/Designer/frontend/libs/studio-components/src/components/StudioInlineEdit/StudioInlineEdit.test.tsx (1)
40-46: Use role-based assertion for description text.The tests are otherwise great, but this one uses
getByText. A role-based assertion keeps it aligned with the testing guideline and the accessible API.♻️ Suggested adjustment
- expect(screen.getByText(description)).toBeInTheDocument(); + expect(screen.getByRole('textbox', { name: label })).toHaveAccessibleDescription(description);
...esigner/frontend/libs/studio-components/src/components/StudioInlineEdit/StudioInlineEdit.tsx
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17608 +/- ##
==========================================
- Coverage 95.98% 95.93% -0.06%
==========================================
Files 2433 2456 +23
Lines 30850 31254 +404
Branches 3598 3644 +46
==========================================
+ Hits 29612 29983 +371
- Misses 923 958 +35
+ Partials 315 313 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Designer/frontend/app-development/features/appSettings/components/TabsContent/Tabs/AboutTab/AppConfigForm/AppConfigForm.test.tsx (1)
22-27:⚠️ Potential issue | 🟡 MinorOpen inline-edit mode before saving (or remove the save click).
StudioInlineEditonly renders the textbox/save button after entering edit mode. These tests either fetch a textbox directly or click the inline-edit save button after editing a regular field, so they’ll fail and/or won’t commit changes. UsegetOptionalInlineEditTextboxfor the inline-edit fields and drop the save click for regular textfields.Proposed fix (apply the same pattern to the other affected tests)
- const anInputField = getOptionalTextbox( - textMock('app_settings.about_tab_homepage_field_label'), - ); - const newValue: string = 'A'; - await user.type(anInputField, newValue); - await user.click(getInlineEditSaveButton()); + const homepage = await getOptionalInlineEditTextbox( + user, + textMock('app_settings.about_tab_homepage_field_label'), + ); + const newValue: string = 'A'; + await user.type(homepage, newValue); + await user.click(getInlineEditSaveButton());- await user.click(getInlineEditSaveButton());Also applies to: 232-236, 252-256, 271-275, 288-292
🤖 Fix all issues with AI agents
In
`@src/Designer/frontend/app-development/features/appSettings/components/TabsContent/Tabs/AboutTab/AppConfigForm/AppConfigForm.tsx`:
- Around line 61-66: The inline-edit for Alt ID is wired to updatedAppConfig.id
but the handler onChangeServiceId updates serviceId, so edits never appear; fix
by making the binding and updater target the same property: either change
onChangeServiceId to update id (setUpdatedAppConfig(old => ({ ...old, id:
newValue }))) or change the input's value to use updatedAppConfig.serviceId and
ensure ApplicationMetadata includes serviceId; apply the same correction to the
second occurrence referenced around the other handler (lines ~143-151) so both
read/write the same property.
🧹 Nitpick comments (1)
src/Designer/frontend/app-development/features/appSettings/components/TabsContent/Tabs/AboutTab/AppConfigForm/AppConfigForm.tsx (1)
201-238: Consider replacing the large commented JSX block with a feature flag or removal.Leaving sizeable commented UI tends to rot; a gated render or a clean removal would be easier to maintain while
#17439is in flight.
...nt/features/appSettings/components/TabsContent/Tabs/AboutTab/AppConfigForm/AppConfigForm.tsx
Outdated
Show resolved
Hide resolved
…e-felter-with-the-addnew-button-pattern
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Designer/frontend/app-development/features/appSettings/components/TabsContent/Tabs/AboutTab/AppConfigForm/AppConfigForm.test.tsx (1)
18-27:⚠️ Potential issue | 🟠 MajorInline‑edit tests still query a textbox without entering edit mode.
StudioInlineEdit only renders the textbox after clicking the view button, so
getOptionalTextbox(...)will fail here. UsegetOptionalInlineEditTextbox(...)instead.✅ Example fix
- const anInputField = getOptionalTextbox( - textMock('app_settings.about_tab_homepage_field_label'), - ); + const anInputField = await getOptionalInlineEditTextbox( + user, + textMock('app_settings.about_tab_homepage_field_label'), + );- const altId = getOptionalTextbox(textMock('app_settings.about_tab_homepage_field_label')); + const altId = await getOptionalInlineEditTextbox( + user, + textMock('app_settings.about_tab_homepage_field_label'), + );- const altId = getOptionalTextbox(textMock('app_settings.about_tab_homepage_field_label')); + const altId = await getOptionalInlineEditTextbox( + user, + textMock('app_settings.about_tab_homepage_field_label'), + );Also applies to: 228-235, 244-255
lassopicasso
left a comment
lassopicasso
left a comment
There was a problem hiding this comment.
Well done! In this first round of review, I’ve focused on the new Studio component StudioInlineEdit. I’ll do a second review later for the places where the component is used.
The naming StudioInlineEdit is a bit vague to me. It sounds like it could support different input types or render arbitrary children. One idea could be to make it support multiple input types via a prop (as children), but then controlled form handling (handleSave, etc.) would need to be done outside the component. We can consider consolidating that later if/when we need to handle different input types with the same UI.
For now, the component is tightly connected to StudioTextfield and only supports editing a string value. I think giving it a more descriptive name would help. Something like StudioInlineTextField seems clearer for this component. What do you think?
I see there is missing a stories file for this new studio component. That is required for studio components that is custom, as far as I know, to be able to test it in storybook.
| icon?: React.ReactNode; | ||
| saveAriaLabel?: string; | ||
| cancelAriaLabel?: string; | ||
| }; |
There was a problem hiding this comment.
I think the following props shouldn't be optional:
label: this is usually always required in a form element.onChange: the core of this component is to pass changes up, and would crash ifonChangeis undefined.
The type of props:
description: to simplify/restrict what we can pass to this prop in this component, could we keep it as astring? I am unsure here, so I'm open for discussion 🙂 Perhaps it is a good idea to keep the type asReactNode, but I don't see the immediately need for this to beReactNode
Perhaps redundant prop:
icon: i suggest for now to import the plus icon directly to the component, instead of passing it down. Since we have kept a design pattern (mostly) where inline toggle form elements have a plus icon when in view mode. What do you think?
| const handleClick = (): void => { | ||
| setInputValue(value); | ||
| setIsEditMode(true); | ||
| }; |
There was a problem hiding this comment.
handleClick is a bit vague in this context, since there is several clickable actions in this file. What about something like openEditMode?
And is there a reason to have setInputValue(value) in this function?
There was a problem hiding this comment.
we need setInputValue(value) to make sure the value is reseted after reset changes.
in this video when i comment setInputValue(value) the the value "test" still shows in field even i reset changes.
Screen.Recording.2026-02-03.at.15.31.55.mov
| <StudioButton | ||
| variant='tertiary' | ||
| className={classes.viewMode} | ||
| onClick={handleClick} | ||
| aria-label={label} | ||
| icon={!value && icon} | ||
| > | ||
| <div className={toggleViewModeContent}> | ||
| <StudioLabel>{label}</StudioLabel> | ||
| <StudioParagraph>{value}</StudioParagraph> | ||
| </div> | ||
| {value && <StudioEditIcon />} | ||
| </StudioButton> |
There was a problem hiding this comment.
This a good solution, but we already have a studio component to solve this: StudioProperty.Button.
Here is an example where it used somewhere else:
<StudioProperty.Button
onClick={() => setEditMode(true)}
property={property}
title={title}
value={value}
className={className}
/>
| <StudioButton icon={<StudioSaveIcon />} onClick={saveValue} aria-label={saveAriaLabel} /> | ||
| <StudioButton | ||
| variant='secondary' | ||
| icon={<StudioCancelIcon />} | ||
| onClick={cancelEditing} | ||
| aria-label={cancelAriaLabel} | ||
| /> |
There was a problem hiding this comment.
We could utilize the new studio component StudioFormActions to keep it is easier to maintain and have a consistent design.
Here is an example from somewhere else:
<StudioFormActions
className={classes.actionButtons}
primary={{
label: t('general.save'),
onClick: handleSave,
disabled: isSaveDisabled,
}}
secondary={{
label: t('general.cancel'),
onClick: handleCancel,
}}
isLoading={false}
iconOnly
/>
Agree 😄 and regarding stories file I wanted actually to wait for it until review is finished to not change again. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/Designer/frontend/libs/studio-components/src/components/StudioInlineTextField/StudioInlineTextField.tsx`:
- Around line 75-86: The save and cancel ARIA labels for the icon-only buttons
are optional and may produce unlabeled controls; update the
StudioInlineTextField component so saveAriaLabel and cancelAriaLabel are always
provided: either make the props required on the StudioInlineTextField props
interface, or set sensible defaults (e.g. "Save" and "Cancel") before passing
them into the StudioFormActions primary/secondary props; ensure the
StudioFormActions call uses those non-null labels so the iconOnly buttons have
accessible names.
🧹 Nitpick comments (2)
src/Designer/frontend/app-development/features/appSettings/components/TabsContent/Tabs/AboutTab/AppConfigForm/AppConfigForm.module.css (1)
15-27: Consider removing commented‑out CSS to keep the module tidy.The commented rules look like leftover experiments and add noise to the stylesheet. If they’re not needed, please delete them to keep the module clean.
🧹 Proposed cleanup
-.inlineTextFieldWithIcon { - display: flex; - align-items: center; - gap: var(--ds-size-3); -} - -/* .inlineTextFieldWithIcon > *:first-child { - flex: 1; -} - -.emptyIcon { - flex-shrink: 0; -} */ +.inlineTextFieldWithIcon { + display: flex; + align-items: center; + gap: var(--ds-size-3); +}src/Designer/frontend/libs/studio-components/src/components/StudioInlineTextField/StudioInlineTextField.test.tsx (1)
26-44: Consider adding a test for the cancel functionality.The save flow is well tested, but there's no test verifying that clicking Cancel reverts changes and closes edit mode without calling
onChange.📝 Suggested additional test case
it('should not call onChange and close edit mode when Cancel is clicked', async () => { const user = userEvent.setup(); const onChange = jest.fn(); renderStudioInlineTextField({ label, value, onChange, saveAriaLabel: 'Save', cancelAriaLabel: 'Cancel', }); await user.click(screen.getByRole('button', { name: label })); await user.clear(screen.getByRole('textbox', { name: label })); await user.type(screen.getByRole('textbox', { name: label }), 'Discarded value'); await user.click(screen.getByRole('button', { name: 'Cancel' })); expect(onChange).not.toHaveBeenCalled(); expect(screen.getByRole('button', { name: label })).toBeInTheDocument(); expect(screen.queryByRole('textbox', { name: label })).not.toBeInTheDocument(); });
...ontend/libs/studio-components/src/components/StudioInlineTextField/StudioInlineTextField.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/Designer/frontend/libs/studio-components/src/components/StudioInlineTextField/StudioInlineTextField.tsx`:
- Around line 40-47: The view-mode root ignores the consumer-supplied className
because StudioProperty.Button is always given only classes.viewMode; update
StudioInlineTextField so when !isEditMode it forwards/merges the incoming
className prop into the StudioProperty.Button call (e.g. combine
classes.viewMode with the component's className using the project's class-merge
utility or simple string concat) so consumers can style the view mode; make the
change in the branch handling isEditMode (where StudioProperty.Button is
rendered) and ensure openEditMode, label, and value are left unchanged.
...ontend/libs/studio-components/src/components/StudioInlineTextField/StudioInlineTextField.tsx
Outdated
Show resolved
Hide resolved
2 participants
Description
CLOSES:#17569
Redesignet
Nøkkelordand thelinkto be inside addButton pluss , and removed theAlternativ IDfield.StudioInlineEditScreen.Recording.2026-02-02.at.13.30.26.mov
Verification
Summary by CodeRabbit
New Features
Tests
Documentation