feat: Add a tab menu for settings on new navigation page #14849
Conversation
|
Warning Rate limit exceeded@mlqn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 11 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 (2)
📝 WalkthroughWalkthroughThis pull request introduces four new localization key-value pairs in the Norwegian language file, adds a new CSS module for styling settings tabs, and creates a new React component named Changes
Possibly related PRs
Suggested labels
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 (
|
github-actions
bot
added
area/ui-editor
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14849 +/- ##
=======================================
Coverage 95.79% 95.79%
=======================================
Files 1924 1925 +1
Lines 25107 25117 +10
Branches 2869 2869
=======================================
+ Hits 24050 24060 +10
Misses 797 797
Partials 260 260 ☔ View full report in Codecov by Sentry. |
![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/components/Settings/SettingsTabs.tsx (1)
8-17: Consider adding type definitions for the tabs array.Adding explicit type definitions would improve code maintainability and type safety.
-const tabs = [ +interface Tab { + icon: React.FC<React.SVGProps<SVGSVGElement>>; + title: string; +} + +const tabs: Tab[] = [ { icon: CompassIcon, title: 'ux_editor.settings.navigation_tab', }, { icon: DatabaseIcon, title: 'ux_editor.settings.data_model_tab', }, ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/language/src/nb.json(1 hunks)frontend/packages/ux-editor/src/components/Settings/SettingsTabs.module.css(1 hunks)frontend/packages/ux-editor/src/components/Settings/SettingsTabs.tsx(1 hunks)frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/ux-editor/src/components/Settings/SettingsTabs.module.css
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
🔇 Additional comments (5)
frontend/packages/ux-editor/src/containers/FormDesignNavigation/FormDesignerNavigation.tsx (2)
7-7: LGTM! Clean import of the new SettingsTabs component.The import path is correct and follows the project's convention for relative imports.
19-19: LGTM! SettingsTabs component is properly integrated.The component is appropriately placed inside the content div, aligning with the UI structure.
frontend/packages/ux-editor/src/components/Settings/SettingsTabs.tsx (2)
1-7: LGTM! Proper imports and dependencies.All necessary imports are included for the component, with appropriate type definitions and styling.
19-42: Tab content panels are missing.The component creates tabs but doesn't include any
StudioTabs.Panelelements to display content when tabs are selected. This suggests the implementation may be incomplete.Is this intentional, with tab content to be added in a future PR? If not, consider adding panel content similar to:
<StudioTabs defaultValue={tabs[0].title}> <StudioTabs.List> {/* existing tab code */} </StudioTabs.List> {tabs.map((tab) => ( <StudioTabs.Panel key={tab.title} value={tab.title}> {/* Content for ${tab.title} */} </StudioTabs.Panel> ))} </StudioTabs>frontend/language/src/nb.json (1)
1916-1918: LGTM! New translation keys match component usage.The translation keys are properly structured under the
ux_editor.settingsnamespace and provide appropriate Norwegian translations for the tab labels and heading.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/packages/ux-editor/src/components/Settings/SettingsTabs.tsx (5)
1-7: Imports are well-organized but could be grouped by type.The imports are properly structured, but it's considered a best practice to group imports by type (React core, external libraries, internal components, styles, etc.) for better readability.
Consider organizing imports in this order:
import type { ReactElement } from 'react'; import React from 'react'; + import { useTranslation } from 'react-i18next'; import { StudioAlert, StudioHeading, StudioTabs } from '@studio/components'; import { CompassIcon, DatabaseIcon } from '@studio/icons'; + import classes from './SettingsTabs.module.css';
13-15: Component lacks documentation.The component is missing JSDoc documentation that would explain its purpose, props, and usage, which would be helpful for other developers.
Add JSDoc documentation to the component:
+/** + * SettingsTabs component provides a tabbed interface for accessing different settings + * categories within the UX editor. Currently includes Navigation and Database settings. + * + * @returns {ReactElement} A React element containing the settings tabs interface + */ export const SettingsTabs = (): ReactElement => { const { t } = useTranslation();
21-31: Enhance accessibility for the tab component.The tabs implementation could benefit from accessibility improvements like aria-labels for better screen reader support.
- <StudioTabs defaultValue={Tabs.Navigation}> + <StudioTabs + defaultValue={Tabs.Navigation} + aria-label={t('ux_editor.settings.tabs_aria_label')} + > <StudioTabs.List> <StudioTabs.Tab value={Tabs.Navigation}> <CompassIcon className={classes.icon} /> {t('ux_editor.settings.navigation_tab')} </StudioTabs.Tab> <StudioTabs.Tab value={Tabs.Database}> <DatabaseIcon className={classes.icon} /> {t('ux_editor.settings.data_model_tab')} </StudioTabs.Tab> </StudioTabs.List>Note: You'll need to add a new translation key 'ux_editor.settings.tabs_aria_label' in your language files.
32-37: Placeholder content could be more descriptive.Both tabs currently display the same generic work-in-progress message. Consider adding more context-specific information about what each tab will eventually contain.
<StudioTabs.Content value={Tabs.Navigation}> - <StudioAlert severity='info'>{t('ux_editor.settings.wip_message')}</StudioAlert> + <StudioAlert severity='info'> + {t('ux_editor.settings.navigation_wip_message')} + </StudioAlert> </StudioTabs.Content> <StudioTabs.Content value={Tabs.Database}> - <StudioAlert severity='info'>{t('ux_editor.settings.wip_message')}</StudioAlert> + <StudioAlert severity='info'> + {t('ux_editor.settings.database_wip_message')} + </StudioAlert> </StudioTabs.Content>Note: You'll need to add new translation keys 'ux_editor.settings.navigation_wip_message' and 'ux_editor.settings.database_wip_message' with more specific messages.
16-40: Consider adding state persistence for tab selection.The component doesn't maintain the selected tab state when the component re-renders. Adding state management would improve the user experience by remembering their tab selection.
import type { ReactElement } from 'react'; -import React from 'react'; +import React, { useState } from 'react'; import { StudioAlert, StudioHeading, StudioTabs } from '@studio/components'; import { useTranslation } from 'react-i18next'; import classes from './SettingsTabs.module.css'; import { CompassIcon, DatabaseIcon } from '@studio/icons'; enum Tabs { Navigation = 'navigation', Database = 'database', } export const SettingsTabs = (): ReactElement => { const { t } = useTranslation(); + const [activeTab, setActiveTab] = useState<Tabs>(Tabs.Navigation); + + const handleTabChange = (value: string) => { + setActiveTab(value as Tabs); + }; return ( <div> <StudioHeading level={2} size='xsmall' className={classes.heading}> {t('ux_editor.settings.other_settings')} </StudioHeading> - <StudioTabs defaultValue={Tabs.Navigation}> + <StudioTabs value={activeTab} onValueChange={handleTabChange}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/language/src/nb.json(1 hunks)frontend/packages/ux-editor/src/components/Settings/SettingsTabs.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/language/src/nb.json
⏰ 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 (3)
frontend/packages/ux-editor/src/components/Settings/SettingsTabs.tsx (3)
8-11: Good use of enum for tab values.Using an enum for tab values provides type safety and makes the code more maintainable. This is a good practice for defining constant values in TypeScript.
5-5:✅ Verification successful
Verify if CSS module file exists.
The component imports a CSS module file, but it's important to ensure this file exists and contains the necessary style classes referenced in the component.
🏁 Script executed:
#!/bin/bash # Description: Check if the CSS module file exists and contains referenced classes # Check if the module file exists if [ -f "frontend/packages/ux-editor/src/components/Settings/SettingsTabs.module.css" ]; then echo "CSS module file exists." # Check for the classes referenced in the component grep -q "\.heading" frontend/packages/ux-editor/src/components/Settings/SettingsTabs.module.css && \ echo "heading class found." || echo "WARNING: heading class not found in CSS module." grep -q "\.icon" frontend/packages/ux-editor/src/components/Settings/SettingsTabs.module.css && \ echo "icon class found." || echo "WARNING: icon class not found in CSS module." else echo "WARNING: CSS module file not found: frontend/packages/ux-editor/src/components/Settings/SettingsTabs.module.css" fiLength of output: 434
CSS Module Verification Successful:
The CSS module filefrontend/packages/ux-editor/src/components/Settings/SettingsTabs.module.cssexists and contains the required.headingand.iconclasses as referenced in the component. No further action is necessary.
3-3:❓ Verification inconclusive
Verify if all Studio components are being used.
The
StudioAlertcomponent is imported from@studio/components, but it might be better to use a more specific component for work-in-progress messages if available in your design system.
🏁 Script executed:
#!/bin/bash # Description: Check if there are more appropriate notification components in the design system # Look for any notification or banner components that might be more suitable for WIP messages fd --type f ".*\.(tsx|ts)" --exec grep -l "export.*Studio.*Banner\|export.*Studio.*Notification" {} \; --exclude "*test*" | sortLength of output: 133
Notification Component Usage – Manual Verification Needed
I reviewed the search results, and no alternative notification components (like a StudioNotification or StudioBanner) were found in the codebase. In the current state, using StudioAlert from
@studio/componentsappears to be the only option available for work-in-progress messages. However, please verify manually with the design team or re-run additional checks if a more specialized component should be used.
- File:
frontend/packages/ux-editor/src/components/Settings/SettingsTabs.tsx(line 3)- Context: The code imports
StudioAlertfrom@studio/components. No alternative notification components were detected in the repository.
mlqn
force-pushed
the
14737-add-a-tab-menu-for-settings
branch
from
9d7110c to
795bc88
Compare
mlqn
force-pushed
the
14737-add-a-tab-menu-for-settings
branch
from
795bc88 to
a9eed42
Compare
Description
Related Issue(s)
Verification
Summary by CodeRabbit