feat: Warn user about deprecated components #14677
Conversation
📝 WalkthroughWalkthroughThis PR implements deprecation support for the Summary component. It adds a new deprecation message in the language file, introduces a CSS rule for deprecated components, and integrates conditional logic via a new utility function ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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 (
|
github-actions
bot
added
area/ui-editor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/utils/component.ts (1)
212-215: Consider improving the deprecated components implementation.The current implementation could be enhanced for better maintainability and type safety:
- Consider moving the deprecated components list to a separate configuration file
- Add JSDoc documentation for the function
- Use a const assertion for type safety
+/** List of deprecated component types */ +const deprecatedComponents = [ComponentType.Summary] as const; + +/** + * Checks if a component type is deprecated + * @param type - The component type to check + * @returns True if the component is deprecated, false otherwise + */ export const isComponentDeprecated = (type: ComponentType) => { - const deprecatedComponents = [ComponentType.Summary]; return deprecatedComponents.includes(type); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/language/src/nb.json(1 hunks)frontend/packages/ux-editor/src/components/Properties/Properties.module.css(1 hunks)frontend/packages/ux-editor/src/components/Properties/Properties.tsx(3 hunks)frontend/packages/ux-editor/src/containers/DesignView/FormTree/FormItem/FormItemTitle/FormItemTitle.module.css(1 hunks)frontend/packages/ux-editor/src/containers/DesignView/FormTree/FormItem/FormItemTitle/FormItemTitle.tsx(2 hunks)frontend/packages/ux-editor/src/utils/component.test.ts(2 hunks)frontend/packages/ux-editor/src/utils/component.ts(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/packages/ux-editor/src/containers/DesignView/FormTree/FormItem/FormItemTitle/FormItemTitle.module.css
- frontend/language/src/nb.json
- frontend/packages/ux-editor/src/components/Properties/Properties.module.css
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (3)
frontend/packages/ux-editor/src/containers/DesignView/FormTree/FormItem/FormItemTitle/FormItemTitle.tsx (1)
14-14: LGTM! Clean implementation of the deprecation visual indicator.The changes effectively integrate the deprecation check and apply the appropriate styling.
Also applies to: 47-47
frontend/packages/ux-editor/src/components/Properties/Properties.tsx (1)
40-44:✅ Verification successful
Verify alert wrapper styling.
The alert implementation looks good, but ensure that the
classes.alertWrapperclass is properly defined in the corresponding CSS module.
🏁 Script executed:
#!/bin/bash # Search for the alertWrapper class definition rg "alertWrapper" -g "*.css" -g "*.scss"Length of output: 208
Alert Wrapper Styling Verified:
ThealertWrapperclass is properly defined infrontend/packages/ux-editor/src/components/Properties/Properties.module.cssand is correctly applied in the component. No further changes are needed.frontend/packages/ux-editor/src/utils/component.test.ts (1)
471-479: LGTM! Well-structured test cases.The test implementation provides good coverage for the
isComponentDeprecatedfunction with clear positive and negative test cases.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14677 +/- ##
=======================================
Coverage 95.76% 95.76%
=======================================
Files 1917 1917
Lines 24972 24979 +7
Branches 2859 2860 +1
=======================================
+ Hits 23915 23922 +7
Misses 798 798
Partials 259 259 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/components/Properties/Properties.test.tsx (1)
273-279: LGTM! The test suite for deprecated components is well-structured.The test case effectively verifies the display of deprecation warnings, using the correct test utilities and following established patterns.
However, consider adding more test cases to cover:
- Components that are not deprecated
- Different types of deprecated components
describe('Deprecated component', () => { it('should show warning when component is deprecated', () => { renderProperties({ formItem: componentMocks[ComponentType.Summary] }); const alert = screen.getByText(textMock('ux_editor.component_properties.deprecated.Summary')); expect(alert).toBeInTheDocument(); }); + + it('should not show warning when component is not deprecated', () => { + renderProperties({ formItem: componentMocks[ComponentType.Input] }); + const alert = screen.queryByText(textMock('ux_editor.component_properties.deprecated.Summary')); + expect(alert).not.toBeInTheDocument(); + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/packages/ux-editor/src/components/Properties/Properties.test.tsx(1 hunks)frontend/packages/ux-editor/src/testing/componentMocks.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (2)
frontend/packages/ux-editor/src/testing/componentMocks.ts (2)
181-184: LGTM! The summary component mock follows established patterns.The mock is correctly implemented using the
commonPropshelper and includes the requiredcomponentRefproperty.
220-220: LGTM! The component mock is correctly exported.The
Summarycomponent mock is properly added to thecomponentMocksexport, maintaining alphabetical order withSummary2.
|
@Ildest og @Annikenkbrathen - kan dere gi tilbakemeldinger basert på bilder og video? Bør noe av designet og tekstinnholdet endres? 🙂 |
Implement feedback on text Co-authored-by: Gørild Døhl <[email protected]>
|
Supert! 👍 Ser fint ut! Jeg bare lurte på om headeren skal fortsatt være i toppen og at meldingen legger seg under den? For meg virker det litt rart om melingen er "utenfor" kolonnen" |
Enig. Endrer det til å være mellom header og id 🙂 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx (1)
44-48: LGTM! Consider adding aria-label for better accessibility.The implementation effectively warns users about deprecated components. The warning is properly positioned and follows React best practices.
Consider adding an aria-label to improve accessibility:
- <StudioAlert size='sm' className={classes.alertWrapper} severity='warning'> + <StudioAlert + size='sm' + className={classes.alertWrapper} + severity='warning' + aria-label={t('ux_editor.component_properties.deprecated.aria_label')} + >frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.test.tsx (1)
154-158: Consider adding more test cases for comprehensive coverage.While the current test effectively verifies the presence of the warning message, consider adding tests for:
- Non-deprecated components (should not show warning)
- Warning message styling (severity and size)
- Warning message accessibility attributes
Example test case for non-deprecated components:
it('should not show warning when component is not deprecated', () => { renderPropertiesHeader({ formItem: componentMocks[ComponentType.Input] }); const alert = screen.queryByText(textMock('ux_editor.component_properties.deprecated.Summary')); expect(alert).not.toBeInTheDocument(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.module.css(1 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.test.tsx(2 hunks)frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.module.css
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Testing
- GitHub Check: Typechecking and linting
🔇 Additional comments (2)
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.tsx (1)
5-5: LGTM!The new imports are correctly organized and necessary for implementing the deprecation warning functionality.
Also applies to: 13-13
frontend/packages/ux-editor/src/components/Properties/PropertiesHeader/PropertiesHeader.test.tsx (1)
18-18: LGTM!The import is properly organized with other test-related imports.
Description
This PR adds alerts to deprecated components. When a deprecated component is selected, an alert is displayed in the properties panel. The component is highlighted in Design View (first picture) by the
--fds-semantic-surface-warning-defaultcolor.The implementation is scalable to add additional components in the future.
deprecated-components.mp4
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Summary by CodeRabbit