fix: simplify studioToggleableTextfield
#14402
Conversation
standeren
added
quality/code
github-actions
bot
added
area/process
standeren
force-pushed
the
clean-up-studio-toggleable-textfield
branch
4 times, most recently
from
faf3d24 to
85a4647
Compare
standeren
force-pushed
the
clean-up-studio-toggleable-textfield
branch
from
85a4647 to
fd87234
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14402 +/- ##
=======================================
Coverage 95.69% 95.69%
=======================================
Files 1889 1887 -2
Lines 24573 24560 -13
Branches 2822 2822
=======================================
- Hits 23515 23503 -12
+ Misses 799 798 -1
Partials 259 259 ☔ View full report in Codecov by Sentry. |
|
Warning Rate limit exceeded@standeren has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request encompasses a comprehensive set of changes across multiple frontend components, focusing on simplifying prop structures, updating translation keys, and refactoring component implementations. The modifications primarily target the Changes
Possibly related PRs
Suggested Reviewers
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 (
|
standeren
force-pushed
the
clean-up-studio-toggleable-textfield
branch
from
422cf06 to
58beda3
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...ry/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.tsx
Fixed
Show fixed
Hide fixed
standeren
force-pushed
the
clean-up-studio-toggleable-textfield
branch
from
06d2774 to
034efb1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (19)
frontend/packages/ux-editor/src/components/config/editModal/EditImage/EditImage.test.tsx (1)
Line range hint
223-228: Add type annotation for consistency.The
userparameter inclickDeleteImageButtonshould be typed for consistency with other similar functions.-const clickDeleteImageButton = async (user) => { +const clickDeleteImageButton = async (user: UserEvent) => { const deleteImageButton = screen.getByRole('button', { name: textMock('ux_editor.properties_panel.images.delete_image_reference_title'), }); await user.click(deleteImageButton); };frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetName/EditLayoutSetName.tsx (1)
28-28: Consider consolidating duplicate translations.The
labelandtitleprops are using the same translation key. Consider either:
- Using different, more specific translation keys if they serve different purposes
- Removing one if they're meant to be identical
label={t('process_editor.configuration_panel_layout_set_name_label')} onBlur={handleOnLayoutSetNameBlur} - title={t('process_editor.configuration_panel_layout_set_name_label')} value={existingLayoutSetName}Also applies to: 30-30
frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetNameRecommendedAction/RecommendedActionChangeName.tsx (2)
Line range hint
27-33: Consider enhancing form submission handling.The form submission could be improved to handle async operations more robustly and clean up error states.
Consider this enhancement:
- const saveNewName = (e: React.FormEvent<HTMLFormElement>) => { + const saveNewName = async (e: React.FormEvent<HTMLFormElement>) => { e.preventDefault(); if (newNameError || newName === '') { return false; } - mutateLayoutSetId({ layoutSetIdToUpdate: bpmnDetails.element.id, newLayoutSetId: newName }); - removeAction(bpmnDetails.element.id); + try { + await mutateLayoutSetId({ + layoutSetIdToUpdate: bpmnDetails.element.id, + newLayoutSetId: newName + }); + removeAction(bpmnDetails.element.id); + } catch (error) { + setNewNameError(t('process_editor.error.update_failed')); + } };
Line range hint
35-37: Consider clearing error state on cancel.The error state should be cleared when cancelling the action to ensure a clean slate for future interactions.
const cancelAction = () => { + setNewNameError(''); + setNewName(''); removeAction(bpmnDetails.element.id); };frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.tsx (1)
91-91: Consider using a stable key for React reconciliation.Using
component.idas the key might cause unnecessary re-renders or state issues if the ID changes during editing. Consider using a more stable identifier or a composite key.- key={component.id} + key={`component-${component.id}-field`}frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.test.tsx (2)
31-38: Enhance custom validation test coverage.Consider adding more test cases for the custom validation:
- Empty input
- Valid input that doesn't trigger validation
- Edge cases (e.g., special characters, maximum length)
it('should run custom validation when value changes', async () => { const user = userEvent.setup(); renderStudioTextField({ customValidation }); await user.click(screen.getByRole('button', { name: value })); - const typedInputValue = 'John'; - await user.type(screen.getByRole('textbox', { name: label }), typedInputValue); - expect(customValidation).toHaveBeenCalledTimes(typedInputValue.length); + // Test empty input + await user.type(screen.getByRole('textbox', { name: label }), ''); + expect(customValidation).toHaveBeenCalledWith(''); + + // Test valid input + await user.type(screen.getByRole('textbox', { name: label }), 'John'); + expect(customValidation).toHaveBeenCalledWith('John'); + + // Test input with special characters + await user.type(screen.getByRole('textbox', { name: label }), '@#$'); + expect(customValidation).toHaveBeenCalledWith('@#$'); });
73-75: Extract magic string to constant.Move the 'John' string to a constant at the top of the file with other test constants.
const value: string = 'value'; const label: string = 'label'; +const testInputValue: string = 'John'; // Later in the test - const inputValue = 'John'; + const inputValue = testInputValue;frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.tsx (3)
78-83: Simplify boolean logic and improve variable naming.The boolean logic for determining what to display could be simplified and made more readable.
Consider this refactor:
- const noUrlProvided = url === undefined && !existingImageUrl; - const currentUrl = !url ? noUrlText : url; - const showValue = currentUrl !== noUrlText || isViewMode; - const value = showValue ? currentUrl : undefined; + const hasNoUrl = !url && !existingImageUrl; + const displayUrl = url || noUrlText; + const shouldShowValue = url || isViewMode; + const value = shouldShowValue ? displayUrl : undefined;
85-96: Consider adding URL validation feedback.The component handles URL input but doesn't provide immediate feedback for invalid URLs.
Consider adding URL validation:
+ const isValidUrl = (url: string) => { + try { + new URL(url); + return true; + } catch { + return false; + } + }; return noUrlProvided ? ( <StudioIconTextfield label={label} value={url} onBlur={onBlur} Icon={LinkIcon} + error={url && !isValidUrl(url) ? t('ux_editor.validation.invalid_url') : undefined} /> ) : ( // ... rest of the code
93-94: Inconsistent prop usage between title and value.The component uses
urldirectly for the title but uses a computedvalue. This might lead to inconsistent display.Consider using the same source for both props:
- title={url} - value={value} + title={value || url} + value={value}frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceiptContent.test.tsx (1)
96-104: LGTM! Consider these minor improvements.The test changes look good and improve readability. Consider these enhancements:
- Move the
existingCustomReceiptLayoutSetIdconstant to the top of the file with other test constants.- Make the test description more specific, e.g., "shows the custom receipt ID in a toggleable textfield when there is an existing custom receipt layout set ID".
frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.stories.tsx (1)
17-20: Consider extracting common test valuesThe story values are identical to StudioToggleableTextfield.stories.tsx. Consider extracting these into shared constants for better maintainability.
// Suggested shared constants in a new file e.g., storyConstants.ts export const STORY_DEFAULTS = { label: 'My awesome label', title: 'My awesome title', value: 'My awesome value', error: '', };frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.tsx (1)
22-34: Consider adding aria-label for better accessibility.While the changes look good, the button could benefit from an explicit aria-label that combines both the value and the action (e.g., "Edit {value}").
- <StudioButton variant='tertiary' className={className} onClick={onClick} {...rest}> + <StudioButton + variant='tertiary' + className={className} + onClick={onClick} + aria-label={`Edit ${rest.value}`} + {...rest} + >frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.test.tsx (1)
21-23: Consider more specific icon tests.While testing for the presence of two icons is good, consider adding more specific tests to ensure:
- The correct icon components are rendered
- The icons have proper styling classes
it('should render the specific icons with proper styling', () => { renderStudioTextfieldToggleView(); const icons = screen.getAllByRole('img', { hidden: true }); expect(icons[0]).toHaveClass('viewModeIconsContainer'); expect(icons[1]).toHaveClass('editIcon'); });frontend/testing/playwright/pages/ProcessEditorPage/CustomReceiptConfig.ts (1)
58-62: LGTM! Enhanced test flexibility with parameterThe change improves test maintainability by accepting a dynamic
layoutSetIdinstead of using a hardcoded translation key. This makes the test more flexible and reusable.Consider adding type validation for the
layoutSetIdparameter to ensure it's a non-empty string:- public async waitForEditLayoutSetIdButtonToBeVisible(layoutSetId: string): Promise<void> { + public async waitForEditLayoutSetIdButtonToBeVisible(layoutSetId: string): Promise<void> { + if (!layoutSetId?.trim()) { + throw new Error('layoutSetId must be a non-empty string'); + } const button = this.page.getByRole('button', { name: layoutSetId, }); await expect(button).toBeVisible(); }frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.tsx (1)
Line range hint
34-71: Consider consolidating error handling logic.The component currently maintains two sources of error state:
- External
errorprop- Internal
errorMessagestate from custom validationConsider consolidating these into a single source of truth to simplify the error handling logic.
-const [errorMessage, setErrorMessage] = useState<string | undefined>(null); +const [validationError, setValidationError] = useState<string | undefined>(null); +const displayError = error || validationError; const runCustomValidation = (event: React.ChangeEvent<HTMLInputElement>): boolean => { const errorValidationMessage = customValidation(event.target.value); if (errorValidationMessage) { - setErrorMessage(errorValidationMessage); + setValidationError(errorValidationMessage); return true; } - setErrorMessage(null); + setValidationError(null); return false; }; const handleBlur = (event: React.FocusEvent<HTMLInputElement>): void => { - if (errorMessage || error) { + if (displayError) { return; }frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.test.tsx (1)
Line range hint
43-105: Test coverage looks comprehensive.The test suite effectively covers:
- Toggle behavior between edit/view modes
- Error handling for invalid inputs
- Validation against JSON schema
- Required field validation
- Change event handling
However, consider adding a test case for the error message display to ensure the user feedback is rendered correctly.
it('should display error message when validation fails', async () => { const user = userEvent.setup(); const error = 'error message'; renderStudioTextfieldSchema({ ...defaultProps, error, }); await user.click(screen.getByRole('button', { name: value })); expect(screen.getByText(error)).toBeInTheDocument(); });frontend/testing/playwright/pages/UiEditorPage.ts (1)
234-236: LGTM! Consider adding error handling.The method is now more precise with exact name matching instead of regex pattern. However, consider adding error handling for cases where the button is not found.
public async deleteOldComponentId(componentId: string): Promise<void> { const button = this.page.getByRole('button', { name: componentId }); + await expect(button).toBeVisible({ timeout: 5000 }); await button.click(); await this.page .getByLabel(this.textMock('ux_editor.modal_properties_component_change_id')) .clear(); }frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.test.tsx (1)
227-231: Simplified button selector functionsThe helper functions have been refactored to use more direct selectors, improving readability and maintainability.
Consider extracting the text mock string to a constant to avoid repetition:
+const EXTERNAL_URL_NOT_ADDED = textMock('ux_editor.properties_panel.images.external_url_not_added'); const getEnterUrlWithPlaceholderButton = () => screen.getByRole('button', { - name: textMock('ux_editor.properties_panel.images.external_url_not_added'), + name: EXTERNAL_URL_NOT_ADDED, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
frontend/language/src/nb.json(1 hunks)frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.module.css(1 hunks)frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.stories.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.test.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioRecommendedNextAction/StudioRecommendedNextAction.stories.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.module.css(2 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.test.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.stories.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.test.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.tsx(2 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.stories.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.test.tsx(4 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.tsx(3 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.test.tsx(6 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/CodeLists.test.tsx(1 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.tsx(2 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/ConfigContent.test.tsx(1 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetName/EditLayoutSetName.test.tsx(2 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetName/EditLayoutSetName.tsx(1 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetNameRecommendedAction/RecommendedActionChangeName.tsx(1 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditTaskId/EditTaskId.test.tsx(5 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditTaskId/EditTaskId.tsx(1 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceipt/CustomReceipt.module.css(0 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceipt/CustomReceipt.test.tsx(4 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceipt/CustomReceipt.tsx(1 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceiptContent.test.tsx(1 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigPanel.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/EditPageId.module.css(0 hunks)frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/EditPageId.test.tsx(4 hunks)frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/EditPageId.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/PageConfigPanel.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/Properties.test.tsx(3 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx(5 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.tsx(2 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.test.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditImage/EditImage.test.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.module.css(0 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.test.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.tsx(3 hunks)frontend/packages/ux-editor/src/containers/DesignView/AddItem/ItemInfo/ItemInfo.tsx(1 hunks)frontend/testing/playwright/pages/ProcessEditorPage/CustomReceiptConfig.ts(1 hunks)frontend/testing/playwright/pages/ProcessEditorPage/ProcessEditorPage.ts(3 hunks)frontend/testing/playwright/pages/UiEditorPage.ts(1 hunks)frontend/testing/playwright/tests/process-editor/process-editor.spec.ts(1 hunks)frontend/testing/playwright/tests/text-editor/text-editor.spec.ts(1 hunks)
💤 Files with no reviewable changes (3)
- frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.module.css
- frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/EditPageId.module.css
- frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceipt/CustomReceipt.module.css
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (60)
frontend/packages/ux-editor/src/components/config/editModal/EditImage/EditImage.test.tsx (1)
9-9: LGTM! Type safety improvements.The addition of UserEvent type annotations enhances type safety and code clarity. The simplified button name in
clickExistingUrlButtonalso improves readability.Also applies to: 190-192, 198-202, 205-211
frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetName/EditLayoutSetName.tsx (1)
28-31: LGTM! Props structure successfully simplified.The refactoring aligns well with the PR objectives, making the component more maintainable while preserving its functionality.
frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.module.css (2)
14-14: Verify the necessity of justify-content with grid layoutThe addition of
justify-content: space-betweenseems redundant since the container already usesgrid-template-columns: auto 1frfor spacing. This might lead to unexpected behavior.Could you clarify if this change is necessary for maintaining consistency with input mode? If not, consider removing it to keep the layout controlled solely by the grid system.
37-42: LGTM! Clean and efficient icon handlingThe removal of the wrapper class and the simplified icon visibility handling through display properties is a good improvement. The hover and focus states are properly maintained.
frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetNameRecommendedAction/RecommendedActionChangeName.tsx (1)
48-48: LGTM! Icon prop change aligns with component simplification.The change from
icontoIconprop follows the PR's objective to improve consistency and simplify component interfaces.frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.tsx (2)
11-11: LGTM!The
EyeIconimport is correctly added and used in theShowCodeListUsagesSourcesModalcomponent.
63-68: Great refactoring of the StudioToggleableTextfield props!The flattened props structure improves readability and maintainability by removing the nested
inputPropsandviewPropsobjects. This aligns well with the PR's objective to simplify the component interface.frontend/packages/ux-editor/src/components/Properties/Properties.test.tsx (3)
54-54: LGTM! Clean refactoring of beforeEach setup.The simplified syntax maintains the same functionality while following JavaScript best practices.
84-84: LGTM! Improved test maintainability.Using
componentMocks[ComponentType.Input].idinstead of a static text mock better reflects the actual component behavior and makes the tests more maintainable.
101-103: LGTM! Improved code formatting and consistency.The multi-line format enhances readability while maintaining consistency with the componentMocks approach.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.tsx (2)
6-6: LGTM!The translation hook import is correctly placed and properly utilized in the component.
90-100: LGTM! Props restructuring improves clarity.The flattened prop structure makes the component's interface more explicit and easier to understand. The direct passing of values and callbacks aligns well with React best practices.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx (6)
14-14: LGTM! Good use of centralized mocks.Using centralized component mocks improves maintainability and consistency across tests.
22-22: LGTM! Clean simplification of beforeEach.Good simplification by directly passing jest.clearAllMocks instead of wrapping it in an arrow function.
25-26: LGTM! Well-structured test refactoring.Good improvements in test maintainability:
- Consistent use of the new renderEditComponentIdRow helper
- More precise assertions that verify both call count and arguments
Also applies to: 32-33, 43-44, 55-56, 61-69, 74-75, 86-87
101-116: LGTM! Clear and well-structured test case.Good improvements in test readability:
- Clear variable naming with idOccupiedByDataType
- Explicit test setup with proper component type specification
124-131: LGTM! Well-structured test constants.Good practice:
- Clear constant naming
- Proper typing with EditComponentIdRowProps
- Use of defaultProps to reduce test duplication
133-136: LGTM! Well-implemented render helper.Excellent implementation:
- Clean setup of default props with override capability
- Proper query client mock data setup
- Type-safe with Partial
frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.test.tsx (2)
10-17: Well-structured test setup!Good practices observed:
- Constants for commonly used values improve maintainability
- Mock functions defined at module level
- Clear mocks after each test to prevent test pollution
92-102: Well-structured test utilities!Good practices observed:
- Properly typed default props
- Clean helper function for component rendering
- Flexible prop override mechanism
frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.module.css (2)
10-10: Consider if the increased margin-top is necessaryThe margin-top has been significantly increased from
spacing-7tospacing-10. This seems like a substantial change that might create excessive vertical space.Could you verify if this large margin is intentional and aligns with the design system guidelines? Consider:
- How this affects the overall component height
- Whether it maintains vertical alignment with adjacent components
✅ Verification successful
The increased margin-top aligns with codebase patterns
The
spacing-10value is consistently used across the codebase for vertical spacing, including similar UI contexts like form elements and panels. Several other components use the same margin-top value, indicating this is an intentional design choice rather than an anomaly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other uses of spacing-10 to understand if this is consistent with other components rg --type css "var\(--fds-spacing-10\)" frontend/Length of output: 2501
4-5: Verify layout impact of container modificationsThe changes to container alignment and padding will affect the component's layout. While the uniform padding creates better consistency, please verify that:
- The reduced padding (from spacing-3 to spacing-2) doesn't make the content feel too cramped
- The flex-start alignment works well with all icon sizes and text lengths
✅ Verification successful
Layout changes align with design system patterns
The reduced padding and flex-start alignment are consistent with similar components across the codebase, particularly input fields and interactive elements. The changes promote better visual consistency and proper icon alignment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components using similar padding patterns to ensure consistency rg --type css "var\(--fds-spacing-[23]\)" frontend/Length of output: 31370
frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.tsx (3)
3-3: LGTM! Import changes align with component usage.The addition of
StudioIconTextfieldimport is consistent with the PR's objective to simplify the component structure.
47-50: LGTM! Good separation of concerns.Extracting the URL editing logic into a separate
EditUrlcomponent improves code organization and maintainability.
39-42: Verify the URL deletion logic.The condition for URL deletion has been modified. Ensure that this change doesn't affect existing functionality where users might want to clear the URL.
✅ Verification successful
URL deletion logic is correctly implemented and well-tested
The URL deletion behavior works as intended:
- Deletes URL only when clearing an existing URL
- Prevents unnecessary deletions when starting with no URL
- Maintains existing URL when no changes are made
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests covering URL deletion scenarios rg -l "onUrlDelete" --type ts --type tsx | xargs rg "test.*delete.*url" -A 5Length of output: 104
Script:
#!/bin/bash # Find test files related to ExternalImage fd "ExternalImage.*test" --type f # Search for isBLurInitialWithEmptyInput implementation ast-grep --pattern 'const isBLurInitialWithEmptyInput = $_' # Find other components using onUrlDelete rg "onUrlDelete" -A 3 -B 3Length of output: 13499
frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceiptContent.test.tsx (1)
96-104: Verify test coverage for view/input mode toggle.Given that the PR's main objective is to enhance consistency between view and input modes of the
studioToggleableTextfield, consider adding test cases to verify:
- Switching between view and input modes
- The behavior of the component in each mode
- The removal of
inputPropsandviewPropsLet's verify the test coverage:
✅ Verification successful
Test coverage is comprehensive and complete
The codebase already has thorough test coverage for the
StudioToggleableTextfieldcomponent:
- View/input mode toggle is tested in both the base component and its implementation
- Component behavior is verified in both modes, including validation and error states
- The removal of
inputPropsandviewPropsis not a concern as they're not used in the codebaseNo additional test cases are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related test cases in the codebase echo "Searching for view/input mode toggle test cases..." rg -l "toggleableTextfield.*view.*mode|toggleableTextfield.*input.*mode" -t tsx -t ts # Search for removed props usage echo "Searching for removed props usage..." rg "inputProps|viewProps" -t tsx -t tsLength of output: 378
Script:
#!/bin/bash # Search for the specific test file content echo "=== CustomReceiptContent test file content ===" cat frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceiptContent.test.tsx echo -e "\n=== Searching for view/input mode toggle test cases ===" rg "toggleableTextfield.*view|toggleableTextfield.*input|toggleableTextfield.*mode" --type-add 'ts:*.{ts,tsx}' -t ts echo -e "\n=== Searching for removed props usage ===" rg "inputProps|viewProps" --type-add 'ts:*.{ts,tsx}' -t tsLength of output: 8250
Script:
#!/bin/bash # Search for the CustomReceiptContent component implementation echo "=== CustomReceiptContent component implementation ===" cat frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceiptContent.tsx # Search for StudioToggleableTextfield usage echo -e "\n=== StudioToggleableTextfield usage ===" rg "StudioToggleableTextfield" --type-add 'ts:*.{ts,tsx}' -t ts -A 5Length of output: 37762
Script:
#!/bin/bash # Check StudioToggleableTextfield test coverage echo "=== StudioToggleableTextfield test file content ===" cat frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.test.tsx # Check CustomReceipt test coverage echo -e "\n=== CustomReceipt test file content ===" cat frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceipt/CustomReceipt.test.tsxLength of output: 11000
frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.tsx (1)
7-7: Great improvement on type safety!The change from a React node to a component type provides better type safety and follows React conventions for component props.
frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.stories.tsx (1)
22-22: LGTM! Consistent with component changesThe story correctly demonstrates the new Icon prop usage.
frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.stories.tsx (1)
18-21: Excellent simplification of props structure!The flattened props structure is more intuitive and aligns well with the PR's objective of simplifying the component interface.
frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.tsx (1)
7-8: LGTM! Type-safe icon handling.Good improvement on type safety by:
- Using
Omitto remove theiconprop- Adding a strongly-typed
Iconprop that expects a component referencefrontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.test.tsx (2)
35-41: LGTM! Well-structured test defaults.Good practice using defaultProps to:
- Centralize common test values
- Make tests more maintainable
- Properly type-check the props
43-44: LGTM! Clean render helper implementation.Good implementation of the render helper:
- Uses Partial type to make all props optional
- Properly merges defaultProps with custom props
frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditTaskId/EditTaskId.tsx (1)
46-50: LGTM! Clean prop structure.Excellent simplification of the component interface by:
- Removing nested inputProps/viewProps
- Using direct prop assignments
- Maintaining proper translation key usage
frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.test.tsx (1)
37-45: LGTM! Well-structured test setup.Good implementation of default props:
- Clear variable declarations
- Properly typed props
- Complete set of required props
frontend/libs/studio-components/src/components/StudioRecommendedNextAction/StudioRecommendedNextAction.stories.tsx (1)
58-58: LGTM! Improved icon prop patternThe change from
icon={<KeyVerticalIcon />}toIcon={KeyVerticalIcon}follows a better pattern by passing the component directly rather than a pre-rendered element. This provides more flexibility and is consistent with React's component composition patterns.frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/EditPageId.tsx (1)
52-55: LGTM! Improved prop structureThe refactoring simplifies the component's API by:
- Removing nested props (viewProps/inputProps) in favor of direct props
- Maintaining proper internationalization with translation keys
- Improving code readability and maintainability
This change aligns well with the PR's objective of enhancing consistency and simplifying the component interface.
frontend/packages/ux-editor/src/containers/DesignView/AddItem/ItemInfo/ItemInfo.tsx (1)
58-58: LGTM! Consistent icon prop patternThe change from
icon={<PencilIcon />}toIcon={PencilIcon}maintains consistency with the new pattern implemented across other components, improving the overall codebase consistency.frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.tsx (2)
7-16: LGTM! Props structure has been improved.The flattening of props enhances type safety and makes the component's API more intuitive. The removal of nested props (
inputPropsandviewProps) in favor of direct props follows React best practices.
74-95: LGTM! Clean and consistent render logic.The render logic maintains a clear separation between view and edit modes while ensuring consistent prop usage across both states.
frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.tsx (1)
Line range hint
28-71: LGTM! Changes align with parent component refactoring.The component has been updated to handle the flattened props structure while maintaining the existing schema validation logic.
frontend/packages/process-editor/src/components/ConfigPanel/ConfigPanel.test.tsx (1)
46-46: LGTM! Improved test reliability.Using
mockBpmnDetails.idinstead of a text mock makes the test more resilient and better reflects the actual component behavior.frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetName/EditLayoutSetName.test.tsx (1)
Line range hint
15-47: LGTM! Consistent test naming.The use of
existingLayoutSetNameMockacross all test cases improves consistency and maintainability.frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.test.tsx (1)
30-40: Props structure has been simplified.The refactoring improves the props structure by flattening nested objects (
inputProps,viewProps) into direct props. This change aligns with React best practices by making the component's API more straightforward and easier to maintain.frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/EditPageId.test.tsx (1)
39-43: Improved test reliability with better selectors.The changes enhance test maintainability by:
- Using role-based queries (
getByRole) instead of less reliable text-based queries- Making selectors more specific with both role and name
Also applies to: 69-73
frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/PageConfigPanel.test.tsx (1)
82-82: Improved test accuracy with dynamic values.The test now uses the actual page name (
newSelectedPage) instead of a static identifier, making it more representative of real usage scenarios.frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.test.tsx (2)
14-14: Improved naming consistency.The variable rename from
codeListNametocodeListTitlebetter reflects its purpose and usage throughout the tests. This change enhances code readability and maintainability.Also applies to: 57-57, 66-66, 86-86, 88-88, 99-99
118-121: Enhanced test reliability with accessibility-friendly selectors.The changes improve test maintainability by:
- Using role-based queries for better accessibility testing
- Making selectors more specific with both role and name attributes
frontend/testing/playwright/pages/ProcessEditorPage/ProcessEditorPage.ts (2)
76-78: Improved selector robustnessThe change from text selector to button role with pattern matching improves reliability and accessibility.
87-87: Consistent label usage across methodsGood consistency in using the same label key
process_editor.configuration_panel_change_task_id_labelacross all methods that interact with the task ID input field.Also applies to: 95-95, 103-103, 110-110, 118-118
frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditTaskId/EditTaskId.test.tsx (1)
49-49: Enhanced test maintainabilityGood improvements in test maintainability:
- Using direct ID references instead of text mocks for buttons
- Consistent usage of role='textbox' with semantic labels
Also applies to: 59-59, 64-66, 81-81, 85-87, 151-151, 155-157, 179-179, 183-185
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.test.tsx (1)
68-68: Consistent component ID usageGood alignment with other test files by using direct component ID reference instead of text mock.
Also applies to: 86-86
frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceipt/CustomReceipt.test.tsx (2)
43-43: Simplified object propertyGood use of object property shorthand syntax for better code readability.
55-55: Consistent ID reference usageGood consistency in using direct layout set ID reference for button names across all test cases.
Also applies to: 80-80, 133-133
frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/ConfigContent.test.tsx (1)
94-94: LGTM! Verify component behavior.The test now uses
mockBpmnDetails.iddirectly instead of a translation string, which aligns with the PR's goal of simplifying the component.Let's verify that this change is consistent with the component's implementation:
frontend/testing/playwright/tests/text-editor/text-editor.spec.ts (1)
62-62: LGTM! Method call updated to be more explicit.The
deleteOldComponentIdmethod now receives a specific component ID parameter, improving type safety and clarity.frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/CodeLists.test.tsx (1)
217-220: LGTM! Improved element selection strategy.The change from
getByTitletogetByRole('textbox')with name is a better practice as it:
- Uses semantic selectors
- Follows accessibility best practices
- Makes tests more resilient to UI changes
frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.test.tsx (2)
12-12: Type import enhancementAdded explicit type import for UserEvent to improve type safety.
234-238: Enhanced type safety and simplified input field handlingThe function has been improved with:
- Added type annotation for UserEvent parameter
- Simplified input field clearing logic
frontend/testing/playwright/tests/process-editor/process-editor.spec.ts (1)
187-189: Updated function call with layout set ID parameterThe test now correctly passes the layout set ID to the visibility check function, improving test specificity.
frontend/language/src/nb.json (1)
699-699: Updated translation key and text for task ID labelThe translation key has been renamed from "change_task_id" to "change_task_id_label" with updated text "Oppgave-ID", making it more descriptive and consistent with the UI.
Let's verify if this translation key is used consistently across the codebase:
✅ Verification successful
Translation key migration verified successfully
The translation key has been completely migrated to "process_editor.configuration_panel_change_task_id_label" and is used consistently across components, tests, and translation files. No instances of the old key format remain in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for both old and new translation keys to ensure complete migration echo "Checking for old translation key usage..." rg "process_editor\.configuration_panel_change_task_id[^_]" echo "Checking for new translation key usage..." rg "process_editor\.configuration_panel_change_task_id_label"Length of output: 2142
...c/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceipt/CustomReceipt.tsx
Show resolved
Hide resolved
| type EditUrlProps = { | ||
| url: string; | ||
| existingImageUrl: string; | ||
| onBlur: (event: React.ChangeEvent<HTMLInputElement>) => void; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type definition could be more precise.
The type definition suggests url is required and of type string, but the component logic handles undefined cases.
Consider updating the type to reflect this:
- url: string;
+ url: string | undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type EditUrlProps = { | |
| url: string; | |
| existingImageUrl: string; | |
| onBlur: (event: React.ChangeEvent<HTMLInputElement>) => void; | |
| }; | |
| type EditUrlProps = { | |
| url: string | undefined; | |
| existingImageUrl: string; | |
| onBlur: (event: React.ChangeEvent<HTMLInputElement>) => void; | |
| }; |
| <Icon aria-hidden className={classes.prefixIcon} /> | ||
| <StudioTextfield size='small' {...rest} className={classes.textfield} /> |
There was a problem hiding this comment.
💡 Codebase verification
Consider making the size prop configurable
The hardcoded size='small' affects all existing usages of StudioIconTextfield across the codebase. Currently, the component is used in various contexts (forms, recommended actions, configuration panels) without specifying a size prop. Making the size configurable would maintain backward compatibility and preserve the component's flexibility.
- Consider either:
- Making size configurable by accepting it as a prop with 'small' as the default
- Or documenting why 'small' size is enforced as a design decision
🔗 Analysis chain
Verify the impact of hardcoded size prop
The addition of hardcoded size='small' might affect existing usages. Consider making it configurable or document the reasoning behind this default.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing StudioIconTextfield usages with size prop
rg -l "StudioIconTextfield" | xargs rg "size.*?['\"]"
Length of output: 795
Script:
#!/bin/bash
# Search for all StudioIconTextfield usages
rg "StudioIconTextfield" -A 2 -B 2
Length of output: 18907
standeren
force-pushed
the
clean-up-studio-toggleable-textfield
branch
from
9a48f01 to
e33e8d1
Compare
There was a problem hiding this comment.
Her er det mye bra. Liker spesielt refaktoreringen av testene og at StudioToggleableTextfield har fått et enklere API. Bra jobbet!
Jeg har en del spørsmål til endringene i ExternalImage.tsx. Er dette noe som kan tas ut i en egen PR?
Husk at siden denne endringen medfører synlige endringer, bør du ikke bruke refactor som prefiks i tittelen. fix er nok mer egnet her. Skill eventuelt alt av endringer i stil/funksjonalitet ut i egne PR-er.
frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.test.tsx
Show resolved
Hide resolved
frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.test.tsx
Outdated
Show resolved
Hide resolved
| import classes from './StudioIconTextfield.module.css'; | ||
|
|
||
| export type StudioIconTextfieldProps = { | ||
| icon: React.ReactNode; | ||
| Icon?: React.ComponentType<React.SVGProps<SVGSVGElement>>; |
There was a problem hiding this comment.
Når vi sender inn ikonet på denne måten, får vi ikke sendt med propper. Er det noen utfordringer med metoden som var brukt her fra før?
There was a problem hiding this comment.
Nei. Tror jeg gjorde det i hovedsak for å slippe en ekstra div. Men var usikker på om det fulgte med noen utfordringer. Kan godt bruke den gamle metoden? 😊
There was a problem hiding this comment.
Jeg ville holdt meg til den gamle, siden vi også gjør det på den måten i StudioButton.
...nents/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.test.tsx
Outdated
Show resolved
Hide resolved
...nents/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.test.tsx
Outdated
Show resolved
Hide resolved
...ents/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.test.tsx
Show resolved
Hide resolved
...or/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx
Outdated
Show resolved
Hide resolved
| const isBLurInitialWithEmptyInput = (existingUrl: string, newUrl: string) => | ||
| newUrl === '' && existingUrl === undefined; | ||
|
|
||
| const isInitialUrlProvided = (url: string, existingImageUrl: string) => | ||
| url !== undefined || !!existingImageUrl; |
There was a problem hiding this comment.
Se kommentar fra Coderabbit. Er det mulig at url er undefined her? I så fall bør det være reflektert i typen.
There was a problem hiding this comment.
Ja, url kommer jo fra useStaten som blir intialisert utifra existingImageUrl som kommer fra komponenten i layouten, så den kan jo være undefined. Men dette vil jo gjelde all bruk av existingImageUrl og url i filen. Skal jeg sette alle typene av de to til å være string | undefined da?
There was a problem hiding this comment.
Jepp. For å unngå å måtte ta hensyn til undefined hele veien, er det vanlig praksis å strukturere koden slik at vi eliminerer situasjonene hvor variabelen kan være undefined på et høyere nivå, men det er jo ikke alltid det er mulig.
| return isInitialUrlProvided(url, existingImageUrl) ? ( | ||
| <StudioToggleableTextfield | ||
| onIsViewMode={setIsViewMode} | ||
| Icon={LinkIcon} | ||
| label={label} | ||
| onBlur={onBlur} | ||
| title={url} | ||
| value={value} | ||
| /> | ||
| ) : ( | ||
| <StudioIconTextfield label={label} value={url} onBlur={onBlur} Icon={LinkIcon} /> | ||
| ); |
There was a problem hiding this comment.
Jeg har vanskeligheter med å forstå hva som skal skje her. Hvorfor laste to forskjellige komponenter avhengig av isInitialUrlProvided? Og hvorfor heter funksjonen initial når den kjøres hver gang komponenten blir lastet med nye verdier?
There was a problem hiding this comment.
Ja, mulig det er dårlig/feil navngivning her. Men slik jeg tolket oppførselen her så vil vi alltid rendre StudioToggleableTextfield dersom existingUrl er satt eller om man har trigget onBlur. Og det impliserer at initiell url er oppgitt. Ellers vil man vise StudioIconTextfield.
Det ble implementert slik for at man skulle bli møtt med input-feltet tilgjengelig med en gang man byttet til Lim inn URL taben, og ikke StudioTextfieldToggleView som er default for StudioToggleableTextfield. Men det er sikkert ikke noe stort problem å bli møtt med visnings-moduset, som skjermbildet viser. Så jeg kan godt fjerne denne kompleksiteten om du tror det er best?
There was a problem hiding this comment.
Slik jeg tolker dette høres det ut som at vi bør gjøre komponenten kontrollerbar. Det vil si å styre tilstanden fra utsiden ved å eksponere isViewMode som en prop. Vi kan bruke den egendefinerte hooken usePropState til å gjøre at komponenten kan være både kontrollert og ukontrollert. Alternativt kan vi håndtere dette på innsiden av komponenten ved sende inn en defaultMode-prop eller noe slikt.
Men hvis dette uansett ikke er så viktig, har i hvert fall ikke jeg noe imot at vi fjerner denne kompleksiteten fullstendig.
There was a problem hiding this comment.
Kan sjekke med @Annikenkbrathen 😊
...packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.tsx
Outdated
Show resolved
Hide resolved
Ja, de endringene i Så dersom endringene knyttet til dette skal skilles ut i en egen PR så vil komponenten oppføre seg litt rart dersom denne blir merget først. Jeg kan jo merge refaktorering av |
standeren
changed the title
studioToggleableTextfieldstudioToggleableTextfield
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.test.tsx (1)
256-300: Consider grouping related test cases.The test suite is comprehensive and covers all edge cases thoroughly. However, readability could be improved by grouping similar test cases using nested describe blocks.
Consider reorganizing the tests like this:
describe('calculateViewValue', () => { const noUrlText = 'No URL Provided'; + describe('with valid URL', () => { it('should return the URL when URL is provided and view mode is false', () => { // ... existing test }); it('should return the URL when URL is provided and view mode is true', () => { // ... existing test }); + }); + describe('with empty or undefined URL', () => { it('should return undefined when URL is empty and view mode is false', () => { // ... existing test }); // ... other empty/undefined tests + }); + describe('with URL matching noUrlText', () => { it('should return undefined when URL is equal to noUrlText and view mode is false', () => { // ... existing test }); it('should return noUrlText when URL is equal to noUrlText and view mode is true', () => { // ... existing test }); + }); });frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.test.tsx (1)
38-39: MoveonChangemock inside describe block.The
onChangemock is defined at file level, which could lead to state leakage between test suites. AlthoughafterEach(jest.clearAllMocks)is present, it's better practice to scope mocks within the describe block.-const onChange = jest.fn(); +describe('StudioIconTextfield', () => { + const onChange = jest.fn();frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.tsx (1)
50-56: Consider combining error checks into a single function.The error checking logic is spread across multiple places. Consider extracting it into a helper function for better maintainability.
+const hasError = (): boolean => Boolean(errorMessage || error); + const handleBlur = (event: React.FocusEvent<HTMLInputElement>): void => { - if (errorMessage || error) { + if (hasError()) { return; }frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.test.tsx (1)
80-88: Consider adding more validation test cases.The custom validation test only covers the error case. Consider adding test cases for:
- Valid input (no error)
- Empty input
- Edge cases (e.g., whitespace)
it('should handle various validation scenarios', async () => { const user = userEvent.setup(); const customError = 'Your name cannot include "test"'; renderStudioTextField({ customValidation: (valueToValidate: string) => valueToValidate.includes('test') ? customError : undefined, }); // Test invalid input await user.click(screen.getByRole('button', { name: value })); await user.type(screen.getByRole('textbox', { name: label }), 'test'); expect(screen.getByText(customError)).toBeInTheDocument(); // Test valid input await user.clear(screen.getByRole('textbox', { name: label })); await user.type(screen.getByRole('textbox', { name: label }), 'valid'); expect(screen.queryByText(customError)).not.toBeInTheDocument(); // Test empty input await user.clear(screen.getByRole('textbox', { name: label })); expect(screen.queryByText(customError)).not.toBeInTheDocument(); });frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx (2)
25-31: Improve test descriptions and organization for better clarity.Consider these improvements to enhance test readability:
- Use consistent terminology in test descriptions (e.g., "button" vs "testIdButton")
- Group related tests using
describeblocks- Make test descriptions more descriptive of the behavior being tested
Example refactor:
- it('should render button ', async () => { + describe('view mode', () => { + it('displays component ID as a clickable button', async () => { - it('should not render the textfield when changing from edit mode to view mode ', async () => { + describe('edit mode', () => { + it('hides the text field when clicking outside', async () => { - it('should call onChange when user change the input in text filed.', async () => { + it('updates component ID when user changes the input and clicks outside', async () => {Also applies to: 42-52, 54-70
124-132: Move constants to the top of the file.Consider moving these constants above the test cases for better code organization. This follows the common pattern of declaring setup variables before their usage and keeps all configuration in one place.
Move these lines to after the initial imports and before the
describeblock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.test.tsx(2 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.module.css(3 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.test.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.test.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.tsx(2 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/ConfigContent.module.css(1 hunks)frontend/packages/process-editor/src/components/ConfigViewerPanel/ConfigViewerPanel.module.css(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx(6 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.test.tsx(3 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/packages/process-editor/src/components/ConfigViewerPanel/ConfigViewerPanel.module.css
- frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/ConfigContent.module.css
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.module.css
- frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.tsx
- frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.test.tsx
🧰 Additional context used
🪛 GitHub Check: Typechecking and linting
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx
[failure] 136-136:
The return type of an async function or method must be the global Promise type. Did you mean to write 'Promise'?
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (6)
frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.test.tsx (2)
5-5: LGTM! Clean import organization.The new imports are well-organized, with types properly separated from value imports.
Also applies to: 12-12
227-238: LGTM! Helper functions improved.The changes enhance type safety and reliability:
- Helper functions are more focused and simpler
- Explicit clear operation before typing prevents potential input issues
- Added type annotation improves type safety
frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.test.tsx (1)
46-47: LGTM! Good type safety improvement.Adding the return type annotation
RenderResulttorenderStudioIconTextfieldimproves type safety and makes the function's contract more explicit.frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.tsx (1)
7-11: LGTM! Clean prop structure simplification.Good job extending
StudioIconTextfieldPropsdirectly instead of nesting props. This makes the component API more intuitive and easier to use.frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.test.tsx (1)
92-98: LGTM! Well-structured default props.Good job centralizing the common props. This reduces duplication and makes the tests more maintainable.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx (1)
134-139:⚠️ Potential issueAdd Promise return type to async function.
The async function is missing its Promise return type, which TypeScript requires for type safety.
const renderEditComponentIdRow = async ( props: Partial<EditComponentIdRowProps> = {}, -): RenderResult => { +): Promise<RenderResult> => { queryClientMock.setQueryData([QueryKey.FormLayouts, org, app, layoutSetName], layouts); return renderWithProviders(<EditComponentIdRow {...defaultProps} {...props} />); };Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Typechecking and linting
[failure] 136-136:
The return type of an async function or method must be the global Promise type. Did you mean to write 'Promise'?
standeren
force-pushed
the
clean-up-studio-toggleable-textfield
branch
from
4db05cc to
5ab0fd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (16)
frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.test.tsx (1)
35-41: Add type annotation for userEvent setup.Consider adding type annotation for better type safety:
- const user = userEvent.setup(); + const user: UserEvent = userEvent.setup();frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.test.tsx (1)
254-298: Consider using test.each for more concise testsWhile the test suite is comprehensive and well-structured, it could be more maintainable using Jest's test.each pattern. This would reduce repetition while maintaining the same coverage.
Here's how you could refactor it:
describe('calculateViewValue', () => { const noUrlText = 'No URL Provided'; test.each([ ['URL provided, view mode false', 'http://example.com', false, 'http://example.com'], ['URL provided, view mode true', 'http://example.com', true, 'http://example.com'], ['Empty URL, view mode false', '', false, undefined], ['Empty URL, view mode true', '', true, 'No URL Provided'], ['Undefined URL, view mode false', undefined, false, undefined], ['Undefined URL, view mode true', undefined, true, 'No URL Provided'], ['URL equals noUrlText, view mode false', 'No URL Provided', false, undefined], ['URL equals noUrlText, view mode true', 'No URL Provided', true, 'No URL Provided'], ])('%s', (_, url, viewMode, expected) => { expect(calculateViewValue(url, noUrlText, viewMode)).toBe(expected); }); });frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.tsx (1)
73-97: Consider extracting view and edit mode renders into separate functions.While the current implementation works well, the render logic could be more organized by extracting the view and edit mode renders into separate functions for better readability.
+ const renderViewMode = () => ( + <StudioProperty.Button + icon={icon} + property={label} + onClick={toggleViewMode} + title={title} + value={value} + /> + ); + + const renderEditMode = () => ( + <StudioIconTextfield + autoFocus + error={error || errorMessage} + icon={icon} + label={label} + onBlur={handleOnBlur} + onChange={handleOnChange} + ref={ref} + title={title} + value={value} + {...rest} + /> + ); + - if (isViewMode) - return ( - <StudioProperty.Button - icon={icon} - property={label} - onClick={toggleViewMode} - title={title} - value={value} - /> - ); - - return ( - <StudioIconTextfield - autoFocus - error={error || errorMessage} - icon={icon} - label={label} - onBlur={handleOnBlur} - onChange={handleOnChange} - ref={ref} - title={title} - value={value} - {...rest} - /> - ); + return isViewMode ? renderViewMode() : renderEditMode();frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.test.tsx (1)
94-104: Consider adding edge case tests.While the current test coverage is good, consider adding tests for:
- Maximum input length validation
- Special character handling
- Empty string validation
frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.tsx (1)
67-67: Consider enhancing error handlingWhile the current implementation works, consider adding type safety for the error callback:
- onChange?.(event); + if (onChange) { + onChange(event); + }This pattern:
- Makes the code more explicit about optional callback handling
- Provides better TypeScript type inference
- Follows consistent error handling patterns
Also applies to: 73-76
frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.tsx (4)
39-42: Document the utility function's purpose.The condition uses
isBLurInitialWithEmptyInput, but its purpose isn't immediately clear from the name or usage. Consider adding a JSDoc comment explaining its role in preventing URL deletion during initial empty input.
76-76: Consider explicit initial state.Based on previous discussions, initializing
isViewModeasundefinedwas agreed upon. However, to improve code clarity, consider adding a comment explaining whyundefinedis the preferred initial state overtrueorfalse.
103-112: Document complex view value calculation.The
calculateViewValuefunction contains non-trivial logic for determining the display value. Consider adding JSDoc documentation explaining:
- The purpose of each parameter
- The logic behind showing/hiding the placeholder text
- Return value scenarios
Example:
/** * Determines the display value for the URL field based on view mode and current URL. * @param url - The current URL value * @param noUrlText - Placeholder text to show when no URL is provided * @param isViewMode - Whether the field is in view mode * @returns The value to display, or undefined to show an empty input */
95-97: Improve function naming.The function name
isBLurInitialWithEmptyInputis unclear and contains a typo ('BLur'). Consider renaming to something more descriptive likeisInitialEmptyUrlInputorshouldIgnoreEmptyInput.frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx (3)
26-31: Consider adding type assertion for better type safety.While the test is well-structured, consider adding a type assertion to ensure the component mock's type safety:
- expect(testIdButton).toHaveTextContent(componentMocks[ComponentType.Input].id); + expect(testIdButton).toHaveTextContent(componentMocks[ComponentType.Input].id as string);
71-79: Enhance test clarity with more explicit assertions.The test verifies the component update but could be more explicit about the expected behavior:
- expect(handleComponentUpdate).toHaveBeenCalledTimes(1); + // Verify that handleComponentUpdate is called exactly once with the complete updated component + expect(handleComponentUpdate).toHaveBeenCalledTimes(1); expect(handleComponentUpdate).toHaveBeenCalledWith({ ...componentMocks[ComponentType.Input], id: newTestId, - }); + }, 'Component should be updated with new ID while preserving other properties');
146-151: Add error handling for query client setup.The render helper should handle potential query client errors:
const renderEditComponentIdRow = async ( props: Partial<EditComponentIdRowProps> = {}, ): Promise<RenderResult> => { - queryClientMock.setQueryData([QueryKey.FormLayouts, org, app, layoutSetName], layouts); + try { + queryClientMock.setQueryData([QueryKey.FormLayouts, org, app, layoutSetName], layouts); + } catch (error) { + console.error('Failed to set query data:', error); + throw new Error('Test setup failed: Could not set form layouts data'); + } return renderWithProviders(<EditComponentIdRow {...defaultProps} {...props} />); };frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditTaskId/EditTaskId.test.tsx (4)
46-50: Enhance initial render test clarity.The test could be more explicit about the component's initial state:
- expect(editTaskIdButton).toBeInTheDocument(); - expect(editTaskIdButton).toHaveTextContent(mockBpmnDetails.id); + expect(editTaskIdButton).toBeInTheDocument(); + expect(editTaskIdButton).toHaveAttribute('aria-label', textMock('process_editor.configuration_panel_change_task_id_label')); + expect(editTaskIdButton).toHaveTextContent(mockBpmnDetails.id, 'Button should display the current task ID');
60-64: Add assertions for complete edit mode state.Consider adding more assertions to verify the complete edit mode state:
expect(editTaskIdInput).toBeInTheDocument(); expect(editTaskIdInput).toHaveValue(mockBpmnDetails.id); + expect(editTaskIdInput).toHaveAttribute('type', 'text'); + expect(editTaskIdInput).not.toBeDisabled(); + expect(screen.queryByRole('button', { + name: textMock('process_editor.configuration_panel_change_task_id_label') + })).not.toBeInTheDocument();
Line range hint
96-152: Enhance validation error assertions.The validation tests could be more thorough in checking error states:
await user.tab(); const errorMessage = await screen.findByText(textMock(expectedError, textArgs)); expect(errorMessage).toBeInTheDocument(); + // Verify error styling and aria attributes + const input = screen.getByRole('textbox'); + expect(input).toHaveAttribute('aria-invalid', 'true'); + expect(errorMessage).toHaveAttribute('role', 'alert');
174-176: Add explicit behavior documentation.Consider adding a comment to clarify the test's purpose:
const input = screen.getByRole('textbox', { name: textMock('process_editor.configuration_panel_change_task_id_label'), }); + // When the user enters the same ID, we should avoid unnecessary updates + // and maintain the current state without triggering any change events
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.stories.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.test.tsx(2 hunks)frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.tsx(2 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.module.css(0 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.test.tsx(0 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.tsx(0 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/index.ts(0 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.test.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.tsx(2 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfield/index.ts(0 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.test.tsx(4 hunks)frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.tsx(3 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/ConfigContent.test.tsx(1 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetName/EditLayoutSetName.test.tsx(1 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetNameRecommendedAction/RecommendedActionChangeName.test.tsx(0 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetNameRecommendedAction/RecommendedActionChangeName.tsx(0 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditTaskId/EditTaskId.test.tsx(4 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceipt/CustomReceipt.test.tsx(4 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceiptContent.test.tsx(1 hunks)frontend/packages/process-editor/src/components/ConfigPanel/ConfigPanel.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/EditPageId.test.tsx(4 hunks)frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/PageConfigPanel.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/Properties/Properties.test.tsx(3 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx(6 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.test.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditImage/EditImage.test.tsx(3 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.test.tsx(8 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.tsx(5 hunks)
💤 Files with no reviewable changes (7)
- frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetNameRecommendedAction/RecommendedActionChangeName.tsx
- frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.test.tsx
- frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetNameRecommendedAction/RecommendedActionChangeName.test.tsx
- frontend/libs/studio-components/src/components/StudioToggleableTextfield/index.ts
- frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/index.ts
- frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.tsx
- frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioTextfieldToggleView/StudioTextfieldToggleView.module.css
🚧 Files skipped from review as they are similar to previous changes (11)
- frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceiptContent.test.tsx
- frontend/packages/process-editor/src/components/ConfigPanel/ConfigPanel.test.tsx
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.test.tsx
- frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.stories.tsx
- frontend/packages/ux-editor/src/components/Properties/Properties.test.tsx
- frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/PageConfigPanel.test.tsx
- frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditLayoutSetName/EditLayoutSetName.test.tsx
- frontend/packages/ux-editor/src/components/Properties/PageConfigPanel/EditPageId.test.tsx
- frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/ConfigContent.test.tsx
- frontend/packages/ux-editor/src/components/config/editModal/EditImage/EditImage.test.tsx
- frontend/packages/process-editor/src/components/ConfigPanel/ConfigEndEvent/CustomReceiptContent/CustomReceipt/CustomReceipt.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (20)
frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.tsx (2)
7-13: LGTM! Type changes improve component flexibility and accessibility.The changes to
StudioIconTextfieldPropsare well-structured:
- Making
iconoptional allows more flexible usage- Adding required
labelenforces accessibility best practices- Using
Overridetype ensures proper prop composition
26-26: Consider making the size prop configurableThe hardcoded
size='small'affects all existing usages ofStudioIconTextfield. Consider either:
- Making size configurable by accepting it as a prop with 'small' as the default
- Or documenting why 'small' size is enforced as a design decision
frontend/libs/studio-components/src/components/StudioIconTextfield/StudioIconTextfield.test.tsx (3)
11-11: LGTM! Proper test cleanup implemented.Adding
afterEach(jest.clearAllMocks)ensures clean test state between runs.
30-33: LGTM! Good test coverage for optional icon.Adding a specific test for the absence of icon improves coverage of the optional prop behavior.
57-59: LGTM! Well-typed test helper implementation.The helper function is well-implemented with:
- Proper return type annotation
- Partial props type for flexibility
- Good use of defaultProps pattern
frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.test.tsx (3)
5-5: LGTM: Import changes are well-structuredThe addition of type imports and the new calculateViewValue function import are properly organized and follow TypeScript best practices.
Also applies to: 12-12
41-43: LGTM: Test modifications improve consistencyThe test cases have been updated to use a simplified approach for accessing the URL button. The changes are consistent across all test cases and maintain the same test coverage while improving code readability.
Also applies to: 145-148, 156-158, 165-168, 183-186, 193-196
227-230: LGTM: Helper functions enhanced with better typing and clarityThe helper functions have been improved with:
- Proper TypeScript type annotations
- Simplified parameter usage
- Explicit input clearing operation
Also applies to: 232-236
frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.tsx (3)
8-14: LGTM! Clean type definition.The simplified type structure using
Overrideutility type is a good improvement. It makes the component's API more straightforward and maintainable.
19-34: LGTM! Clean state management.The flattened props structure and state management look good. The error handling properly combines both component error and validation error states.
55-70: LGTM! Well-structured event handlers.The event handlers are well-implemented with proper error handling and event forwarding.
frontend/libs/studio-components/src/components/StudioToggleableTextfield/StudioToggleableTextfield.test.tsx (1)
10-17: LGTM! Well-structured test setup.The test constants and mock cleanup are well-organized, improving test maintainability.
frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.test.tsx (1)
30-40: LGTM! Well-structured schema validation tests.The test setup and default props are well-organized, with good coverage of schema validation scenarios.
frontend/libs/studio-components/src/components/StudioToggleableTextfieldSchema/StudioToggleableTextfieldSchema.tsx (3)
8-8: Great improvement in type definitions!The use of the
Overrideutility type is a cleaner approach to extendingStudioToggleableTextfieldProps. This change improves type safety and makes the component's API more maintainable.Also applies to: 15-24
32-39: Props restructuring improves API clarity!The flattened props structure aligns well with the PR's goal of simplifying the component's API. Direct prop access makes the component more intuitive to use.
15-24: Verify the impact of API changesThe removal of nested props (
inputProps,viewProps) and flattening of the API is a breaking change. Let's verify all usages to ensure they've been updated:Also applies to: 32-39
✅ Verification successful
API changes are safe to proceed ✅
The verification shows that all existing usages of
StudioToggleableTextfieldSchemaare already using the new flattened API structure. No instances of the deprecated nested props (inputProps,viewProps) were found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of inputProps or viewProps rg -l "inputProps|viewProps" "frontend/" -g "!*.test.*" -g "!*.stories.*" # Find all imports of StudioToggleableTextfieldSchema to check usage rg -l "import.*StudioToggleableTextfieldSchema" "frontend/"Length of output: 954
Script:
#!/bin/bash # Check if inputProps/viewProps are related to StudioToggleableTextfieldSchema rg "inputProps|viewProps" --context 3 frontend/packages/ux-editor-v3/src/components/config/Expressions/ExpressionContent/ExpressionPreview/ExpressionPreview.tsx frontend/packages/ux-editor-v3/src/components/config/Expressions/ExpressionContent/ExpressionPreview/SimpleExpressionPreview.tsx frontend/packages/ux-editor/src/components/config/editModal/EditImage/LocalImage/ImportImage/AddImageFromLibrary/ChooseFromLibrary/ImageLibraryPreview.tsx frontend/packages/ux-editor/src/components/Preview/Preview.tsx # Check usage in EditComponentIdRow rg "StudioToggleableTextfieldSchema" --context 5 frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.tsxLength of output: 9163
frontend/packages/ux-editor/src/components/config/editModal/EditImage/ExternalImage/ExternalImage.tsx (3)
12-12: LGTM: Type definitions properly handle undefined values.The updated type definitions correctly reflect that
existingImageUrlcan be undefined, addressing previous review feedback. The newEditUrlPropstype is well-structured.Also applies to: 68-72
47-50: LGTM: Good component extraction.The URL editing logic has been cleanly extracted into a separate
EditUrlcomponent, improving code organization and reusability.
81-92: Add test coverage for component rendering logic.The component conditionally renders different textfields based on URL availability. As mentioned in past reviews, this logic would benefit from test coverage demonstrating the expected behavior in various scenarios.
Run the following script to check existing test coverage:
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx (1)
Line range hint
1-24: LGTM! Well-structured test setup.The imports and test setup follow testing-library best practices with proper type imports and mock organization.
| const idOccupiedByDataType = 'idOccupiedByDataType'; | ||
| queryClientMock.setQueryData([QueryKey.AppMetadata, org, app], { | ||
| dataTypes: [{ id: 'newTestId' }], | ||
| }); | ||
| await studioRender({ | ||
| component: { | ||
| type: ComponentType.FileUpload, | ||
| id: '', | ||
| itemType: 'COMPONENT', | ||
| description: 'test', | ||
| displayMode: 'test', | ||
| hasCustomFileEndings: false, | ||
| maxFileSizeInMB: 100, | ||
| maxNumberOfAttachments: 2, | ||
| minNumberOfAttachments: 1, | ||
| }, | ||
| }); | ||
| const testIdButton = screen.getByRole('button', { name: textMock('ux_editor.id_identifier') }); | ||
| dataTypes: [{ id: idOccupiedByDataType }], | ||
| }); | ||
| await renderEditComponentIdRow({ | ||
| component: componentMocks[ComponentType.FileUpload], | ||
| }); | ||
| const testIdButton = screen.getByRole('button', { | ||
| name: textMock('ux_editor.modal_properties_component_change_id'), | ||
| }); | ||
| await user.click(testIdButton); | ||
| const textField = screen.getByRole('textbox', { | ||
| name: textMock('ux_editor.modal_properties_component_change_id'), | ||
| }); | ||
| await user.clear(textField); | ||
| await user.type(textField, 'newTestId'); | ||
| await user.type(textField, idOccupiedByDataType); | ||
| await user.click(document.body); | ||
| expect( | ||
| screen.getByText(textMock('ux_editor.error_component_id_exists_as_data_type')), | ||
| ).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for queryClientMock state.
The test modifies global query client state but doesn't clean it up:
it('should show error message when id of an attachment component type has duplicate id', async () => {
const user = userEvent.setup();
const idOccupiedByDataType = 'idOccupiedByDataType';
+ const originalData = queryClientMock.getQueryData([QueryKey.AppMetadata, org, app]);
queryClientMock.setQueryData([QueryKey.AppMetadata, org, app], {
dataTypes: [{ id: idOccupiedByDataType }],
});
+ afterEach(() => {
+ queryClientMock.setQueryData([QueryKey.AppMetadata, org, app], originalData);
+ });
// ... rest of the test
});Committable suggestion skipped: line range outside the PR's diff.
standeren
force-pushed
the
clean-up-studio-toggleable-textfield
branch
from
1466a0e to
949c55e
Compare
standeren
force-pushed
the
clean-up-studio-toggleable-textfield
branch
from
1828979 to
e446316
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.test.tsx (1)
176-181: LGTM! Improved helper function signature.Good improvement to use a generic
titleparameter name while maintaining consistency with the translation parameter.Consider adding JSDoc documentation to clarify the function's purpose and parameter usage:
+/** + * Gets the code list accordion element by its title + * @param title - The title of the code list + * @returns The accordion element + */ const getCodeListAccordion = (title: string) => {frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx (1)
75-83: Consider adding negative test cases.The test suite has good coverage for success and basic error cases, but could benefit from additional edge cases:
- Test with special characters in the ID
- Test with very long IDs
- Test with whitespace-only IDs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/language/src/nb.json(1 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.test.tsx(9 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/CodeLists.test.tsx(1 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.tsx(2 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx(5 hunks)frontend/testing/playwright/pages/ProcessEditorPage/ProcessEditorPage.ts(3 hunks)frontend/testing/playwright/pages/UiEditorPage.ts(1 hunks)frontend/testing/playwright/tests/process-editor/process-editor.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/CodeLists.test.tsx
- frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.tsx
- frontend/testing/playwright/tests/process-editor/process-editor.spec.ts
- frontend/testing/playwright/pages/ProcessEditorPage/ProcessEditorPage.ts
- frontend/language/src/nb.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (10)
frontend/testing/playwright/pages/UiEditorPage.ts (1)
235-238: LGTM! Improved button selection logic.The updated implementation uses a more reliable and maintainable approach by selecting the button based on the translation key instead of a regex pattern. This change:
- Reduces brittleness by removing regex dependency
- Improves maintainability by using the translation system
- Aligns with Playwright's best practices for role-based selectors
frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.test.tsx (6)
15-15: LGTM! Improved variable naming.The rename from
codeListNametocodeListTitlebetter reflects the semantic meaning of the variable.
57-58: LGTM! Consistent variable usage.The test assertion correctly uses the renamed variable.
70-71: LGTM! Consistent parameter naming.The forEach loop correctly uses the
titleparameter, maintaining consistency with the new naming convention.
88-90: LGTM! Updated translation parameter.The translation template correctly uses the renamed parameter.
98-101: LGTM! Updated button assertion.The button role assertion correctly uses the renamed variable.
120-122: LGTM! Updated function call and assertion.The test correctly uses the renamed variable in both the function call and assertion.
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/EditComponentIdRow/EditComponentIdRow.test.tsx (3)
2-2: LGTM! Improved type safety and test isolation.Good improvements:
- Added type safety with RenderResult import
- Enhanced test isolation by clearing queryClient state
Also applies to: 26-26
119-139: Add cleanup for queryClientMock state.The test modifies global query client state but doesn't clean it up after the test.
it('should show error message when id of an attachment component type has duplicate id', async () => { const user = userEvent.setup(); const idOccupiedByDataType = 'idOccupiedByDataType'; + const originalData = queryClient.getQueryData([QueryKey.AppMetadata, org, app]); queryClient.setQueryData([QueryKey.AppMetadata, org, app], { dataTypes: [{ id: idOccupiedByDataType }], }); + afterEach(() => { + queryClient.setQueryData([QueryKey.AppMetadata, org, app], originalData); + }); // ... rest of test });
150-155: Add return type annotation to the render helper function.The function has a Promise return type from the type parameter, but it's good practice to explicitly annotate it.
- const renderEditComponentIdRow = async ( + const renderEditComponentIdRow = async ( props: Partial<EditComponentIdRowProps> = {}, - ): Promise<RenderResult> => { + ): Promise<RenderResult> => { queryClient.setQueryData([QueryKey.FormLayouts, org, app, layoutSetName], layouts); return renderWithProviders(<EditComponentIdRow {...defaultProps} {...props} />, { queryClient }); };
standeren
force-pushed
the
clean-up-studio-toggleable-textfield
branch
from
dd31bcf to
95dbeb8
Compare
Description
This PR will make sure the both view and input mode looks as equal as possible. I have removed the
inputPropsand theviewPropsto make the component less adjustable from the outside.As a part of the above, I have also removed the
childrenprop inStudioTextfieldToggleViewand replaced it withvalue.I have another PR that adapts the
StudioDisplayTileto align more with theStudioTextfieldToggleViewThis change gives us a more consistent view across Studio:
Today:
Skjermopptak.2025-01-10.kl.15.20.46.mov
In this PR:
Skjermopptak.2025-01-10.kl.15.19.14.mov
Verification
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
Language Updates
UI Components
StudioTextfieldToggleViewcomponentStyling
Testing
Performance
Usability