feat: 14951 create new sidegruppe level as an alternate view #15054
Conversation
…-page' of https://github.com/Altinn/altinn-studio into 14949-create-new-sideoppsett-header-to-use-in-utforming-page
📝 WalkthroughWalkthroughThis pull request introduces a new type called Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (3)
frontend/language/src/nb.json (1)
1947-1950: Localization Keys: Typos & ConsistencyThe new keys support the alternate view for side groups as intended by the PR. However, please review the following details:
- Key
"ux_editor.side_oppsett_add_group_division": The value is"Add gruppeinndeling". Consider if the mix of English ("Add") and Norwegian is intentional. It might be preferable to use a fully localized string such as"Legg til gruppeinndeling".- Key
"ux_editor.side_oppsett_perfome_another_task": The value includes typographical errors –"Utofom en anne oppgave"should likely be corrected to"Utfør en annen oppgave".- Keys
"ux_editor.side_oppsett_header"and"ux_editor.side_oppsett_remove_group_division": These values are appropriate and consistent.Please update the text as needed for clarity and full localization.
frontend/packages/ux-editor/src/containers/DesignView/DesignView.tsx (1)
156-201: Implement error handling for missing layout data.The
displayGroupAccordionsfunction has good structure but could benefit from improved error handling:
- The function checks if
layoutexists before proceeding, but lacks explicit error messaging when a layout is missing- The nested layout could be undefined, but there's no fallback rendering
Additionally, there's some code duplication between this function and the original
displayPageAccordionsfunction. Consider refactoring common logic into a shared helper function.- const displayGroupAccordions = !mockGroups.length - ? null - : mockGroups.map((group) => ( + const displayGroupAccordions = !mockGroups.length + ? null + : mockGroups.map((group) => { + // Check if group has valid pages before rendering + if (!group.pages || group.pages.length === 0) { + return null; + } + return ( <div key={group.name}> <div className={classes.groupHeaderWrapper}> <div className={classes.container}> <FolderIcon aria-hidden className={classes.liftIcon} /> <StudioHeading level={3} size='2xs'> {group.name} </StudioHeading> </div> <DragVerticalIcon aria-hidden className={classes.rightIcon} /> </div> {group.pages.map((page) => { const layout = formLayoutData?.find((formLayout) => formLayout.page === page.id); - if (!layout) return null; + if (!layout) { + console.warn(`Layout not found for page: ${page.id}`); + return null; + } const isInvalidLayout = duplicatedIdsExistsInLayout(layout.data); return ( <div key={page.id} className={classes.groupAccordionWrapper}> <PageAccordion key={page.id} pageName={page.id} isOpen={page.id === selectedFormLayoutName} onClick={() => handleClickAccordion(page.id)} isInvalid={isInvalidLayout} hasDuplicatedIds={layoutsWithDuplicateComponents.duplicateLayouts.includes( page.id, )} navigationMenuClassName={classesPageAccordion.customNavigationMenu} > {page.id === selectedFormLayoutName && ( <FormLayout layout={layout.data} isInvalid={isInvalidLayout} duplicateComponents={layoutsWithDuplicateComponents.duplicateComponents} /> )} </PageAccordion> </div> ); })} </div> - )); + ); + });frontend/packages/ux-editor/src/containers/DesignViewNavigation/DesignViewNavigation.test.tsx (1)
58-60: Consider adding more test variations.The
renderDesignViewNavigationhelper function is a good practice, but it could be enhanced to support different test scenarios, such as rendering with different props or in different states.-const renderDesignViewNavigation = () => { - return renderWithProviders()(<DesignViewNavigation />); -}; +const renderDesignViewNavigation = (props = {}) => { + return renderWithProviders()(<DesignViewNavigation {...props} />); +};Also, consider adding tests for:
- Menu item actions when implemented
- Keyboard navigation within the dropdown menu
- Behavior when a different feature flag configuration is used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
frontend/language/src/nb.json(1 hunks)frontend/libs/studio-components/src/components/StudioSectionHeader/StudioSectionHeader.tsx(2 hunks)frontend/packages/shared/src/types/api/dto/PageModel.ts(1 hunks)frontend/packages/shared/src/types/api/dto/PagesModel.ts(1 hunks)frontend/packages/shared/src/utils/featureToggleUtils.test.ts(2 hunks)frontend/packages/shared/src/utils/featureToggleUtils.ts(1 hunks)frontend/packages/ux-editor/src/containers/DesignView/DesignView.module.css(1 hunks)frontend/packages/ux-editor/src/containers/DesignView/DesignView.tsx(2 hunks)frontend/packages/ux-editor/src/containers/DesignView/PageAccordion/PageAccordion.module.css(1 hunks)frontend/packages/ux-editor/src/containers/DesignView/PageAccordion/PageAccordion.tsx(3 hunks)frontend/packages/ux-editor/src/containers/DesignViewNavigation/DesignViewNavigation.module.css(1 hunks)frontend/packages/ux-editor/src/containers/DesignViewNavigation/DesignViewNavigation.test.tsx(1 hunks)frontend/packages/ux-editor/src/containers/DesignViewNavigation/DesignViewNavigation.tsx(1 hunks)frontend/packages/ux-editor/src/containers/DesignViewNavigation/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
frontend/packages/shared/src/types/api/dto/PagesModel.ts (1)
frontend/packages/shared/src/types/api/dto/PageModel.ts (2)
PageModel(1-3)GroupModel(5-10)
frontend/packages/shared/src/utils/featureToggleUtils.test.ts (1)
frontend/packages/shared/src/utils/featureToggleUtils.ts (1)
shouldDisplayFeature(34-46)
frontend/packages/ux-editor/src/containers/DesignView/PageAccordion/PageAccordion.tsx (1)
frontend/packages/ux-editor/src/containers/DesignView/PageAccordion/NavigationMenu/NavigationMenu.tsx (1)
NavigationMenu(23-89)
frontend/packages/ux-editor/src/containers/DesignView/DesignView.tsx (2)
frontend/packages/ux-editor/src/containers/DesignView/PageAccordion/PageAccordion.tsx (1)
PageAccordion(39-109)frontend/packages/ux-editor/src/containers/DesignViewNavigation/DesignViewNavigation.tsx (1)
DesignViewNavigation(8-57)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Testing
🔇 Additional comments (27)
frontend/packages/ux-editor/src/containers/DesignView/PageAccordion/PageAccordion.module.css (1)
35-37: Looks good - new CSS class for navigation menu stylingThe addition of the
.customNavigationMenuclass with a neutral background color aligns well with the purpose of creating a new side group level view. This follows the existing styling patterns in the file and uses the design system's color variables appropriately.frontend/packages/ux-editor/src/containers/DesignViewNavigation/index.ts (1)
1-1: LGTM - proper module exportThis follows the standard pattern for React/TypeScript module organization, making the
DesignViewNavigationcomponent easily importable from other parts of the codebase.frontend/packages/shared/src/types/api/dto/PageModel.ts (1)
5-10: Good type definition for the new GroupModelThe
GroupModeltype is well-structured for the side group feature, providing the necessary properties for grouping pages together. The design allows for:
- Named groups with the required
nameproperty- Nested pages with the
pagesarray- Type categorization with the optional
typeproperty- Completion tracking with the optional
markWhenCompletedflagThis implementation provides a solid foundation for the hierarchical structure needed by the side group feature.
frontend/packages/shared/src/utils/featureToggleUtils.ts (1)
16-16: Feature flag for controlled rollout - good approachAdding the
TaskNavigationPageGroupsfeature flag is a prudent approach that allows for controlled rollout of the side group feature. This follows the established pattern in the codebase for feature flagging and ensures the feature can be toggled on/off without code changes.Since this isn't added to
defaultActiveFeatures, it will be off by default until explicitly enabled, which is appropriate for a new feature.frontend/packages/ux-editor/src/containers/DesignViewNavigation/DesignViewNavigation.module.css (1)
1-4: LGTM! Clean CSS implementation using design system variables.The CSS implementation appropriately uses CSS variables (
--fds-semantic-surface-neutral-subtlefor background color and--properties-width-fractionfor flex property) which is good for maintainability and consistency across the application. The selector is properly scoped to the component using CSS modules.frontend/packages/shared/src/types/api/dto/PagesModel.ts (2)
1-1: Appropriate import addition for the new model type.The import statement has been correctly updated to include
GroupModelalongsidePageModel, which is necessary for the type extension below.
5-5: Well-structured type extension to support group functionality.The
PagesModeltype has been properly extended with agroupsarray property of typeGroupModel[], aligning with the PR objective to create a new side group level as an alternate view. This type structure correctly establishes the relationship between pages and groups.frontend/libs/studio-components/src/components/StudioSectionHeader/StudioSectionHeader.tsx (3)
17-17: Good enhancement to component props interface.Adding an optional menu prop to the
StudioSectionHeaderPropstype enhances the component's flexibility while maintaining backward compatibility with existing usages.
22-22: Props destructuring correctly updated.The destructuring of props has been properly updated to include the new
menuproperty, maintaining consistency with the component interface change.
39-39: Appropriate placement of the menu in component rendering.The menu is conditionally rendered at an appropriate position in the component layout (after the help text), allowing for seamless integration of navigation options.
frontend/packages/shared/src/utils/featureToggleUtils.test.ts (3)
41-49: Well-structured test cases for localStorage feature toggle.The tests follow the established pattern in the file and properly verify both positive and negative scenarios for the new
taskNavigationPageGroupsfeature flag when set in localStorage.
102-110: Consistent test coverage for URL-based feature toggle.These test cases maintain the same pattern as other feature flag tests, ensuring the
taskNavigationPageGroupsfeature flag works correctly when specified in the URL parameters.
40-110:❓ Verification inconclusive
Verify integration with the FeatureFlag enum.
The tests check for a new enum value
FeatureFlag.TaskNavigationPageGroups, but there's no evidence in the provided files showing this enum value has been added to the FeatureFlag enum.
🏁 Script executed:
#!/bin/bash # Check if the TaskNavigationPageGroups flag is defined in the featureToggleUtils.ts file rg -A 5 -B 5 "export enum FeatureFlag" --include "featureToggleUtils.ts"Length of output: 514
Manual Verification Needed: Confirm Integration of TaskNavigationPageGroups in FeatureFlag Enum
The test cases in
frontend/packages/shared/src/utils/featureToggleUtils.test.tsreference a new enum value—FeatureFlag.TaskNavigationPageGroups—but our initial automated search did not confirm its presence in the FeatureFlag enum. Please manually verify that this enum value is correctly added to the enum (typically defined in a file likefrontend/packages/shared/src/utils/featureToggleUtils.tsor a similar location).
- Ensure the
FeatureFlagenum includesTaskNavigationPageGroups.- If missing, add it or update the tests accordingly.
frontend/packages/ux-editor/src/containers/DesignView/PageAccordion/PageAccordion.tsx (2)
23-23: Good addition of optional property for enhanced styling flexibilityThe optional
navigationMenuClassNameproperty allows for contextual styling of the navigation menu, which is helpful for the new side group level feature.
67-107: Structural improvement with proper component nestingThe update properly wraps the Accordion.Item within an Accordion component, which follows the component's intended usage pattern. The conditional class name handling for the navigation menu is also well implemented.
frontend/packages/ux-editor/src/containers/DesignViewNavigation/DesignViewNavigation.tsx (2)
8-11: Good component structure with state managementThe component is well-structured with proper state management for the dropdown menu. The use of translation hooks ensures internationalization support.
37-49: Implementation needed for dropdown menu actionsThe dropdown menu items have undefined onClick handlers, as noted in the comment. While this is clearly marked as a placeholder, ensure these are implemented in a follow-up task.
Consider creating specific tickets for implementing each action to ensure they don't get overlooked in the development process.
frontend/packages/ux-editor/src/containers/DesignView/DesignView.module.css (2)
17-22: Well-structured CSS for group header layoutThe groupHeaderWrapper class appropriately uses flex layout with proper spacing variables to ensure consistent styling across the application.
24-42: Consistent styling approach for icon and container elementsThese new CSS classes follow the project's styling patterns, utilizing design system variables for spacing and sizing. The groupAccordionWrapper properly defines margins for nested group content.
frontend/packages/ux-editor/src/containers/DesignView/DesignView.tsx (3)
11-11: Good imports organization to support new functionality.The new imports properly support the added features - icons for the UI elements, the
DesignViewNavigationcomponent, feature toggle utilities, and styling for the page accordion. This demonstrates good preparation for implementing the new side group level feature.Also applies to: 18-18, 23-25
203-204: Good feature flag implementation.The code correctly uses feature flags to conditionally render new UI elements. This is a good practice for progressive feature deployment and testing.
209-217: Clean conditional rendering implementation.The code logic for displaying either group accordions or page accordions is clean and readable. The conditional display of the
DesignViewNavigationcomponent based on the feature flag is also well-implemented.frontend/packages/ux-editor/src/containers/DesignViewNavigation/DesignViewNavigation.test.tsx (5)
1-7: Appropriate test imports and setup.The test file uses the right imports for testing-library and includes the component under test. The use of mocks for internationalization is particularly good practice.
8-12: Good test for basic rendering.The first test case properly verifies that the component renders with the correct header text, which is essential for ensuring that internationalization works correctly.
14-24: Good accessibility testing.These tests properly verify both the menu button's title and its accessibility attribute (aria-haspopup). This is excellent practice for ensuring the component meets accessibility standards.
26-38: Robust interaction testing.This test properly verifies that the dropdown menu opens when clicking the menu button. The test correctly sets up user interaction, checks the initial state, performs the action, and then verifies the expected outcome.
40-55: Complete dropdown behavior testing.This test verifies that the dropdown closes when clicking outside, which is an important UX pattern. The use of
waitForto handle asynchronous UI updates is particularly good practice.
frontend/packages/ux-editor/src/containers/DesignView/DesignView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/ux-editor/src/testing/layoutMock.ts (1)
179-182: LGTM - Initial mock groups array is appropriate.The addition of the empty
groupsarray topagesModelMockaligns with the PR objective of creating a new side group level as an alternate view. This change provides the foundation for testing the new group functionality.Consider adding some mock group data to this file to facilitate comprehensive testing of the new side group functionality:
export const pagesModelMock: PagesModel = { pages: [{ id: layout1NameMock }, { id: layout2NameMock }], - groups: [], + groups: [ + { + id: 'group-1', + name: 'Group 1', + pages: [layout1NameMock] + } + ], };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/packages/ux-editor/src/testing/layoutMock.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Testing
JamalAlabdullah
changed the base branch from
main
to
14949-create-new-sideoppsett-header-to-use-in-utforming-page
…oppsett-header-to-use-in-utforming-page
…der-to-use-in-utforming-page' into 14951-create-new-sidegruppe-level-as-an-alternate-view
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15054 +/- ##
==========================================
+ Coverage 95.93% 95.94% +0.01%
==========================================
Files 1996 1996
Lines 25869 25890 +21
Branches 2921 2928 +7
==========================================
+ Hits 24817 24841 +24
+ Misses 788 787 -1
+ Partials 264 262 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…-page' into 14951-create-new-sidegruppe-level-as-an-alternate-view
github-actions
bot
added
the
quality/testing
|
We will wait for this one until this is merged |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/ux-editor/src/containers/DesignView/DesignView.tsx (2)
154-206: Consider refactoring to reduce code duplication.The
displayGroupAccordionsfunction duplicates logic fromdisplayPageAccordions. Consider extracting common rendering logic to a separate function to improve maintainability.// Add a helper function to render page content +const renderPageContent = (pageId, layout, isInvalidLayout, duplicateLayouts, duplicateComponents) => { + return pageId === selectedFormLayoutName ? ( + <FormLayout + layout={layout.data} + isInvalid={isInvalidLayout} + duplicateComponents={duplicateComponents} + /> + ) : null; +}; // Then use this in both displayPageAccordions and displayGroupAccordions
174-177: Improve error handling for missing layouts.Using
console.warnis good for development, but consider providing better user feedback or fallback behavior when a layout is not found for a page.-if (!layout) { - console.warn(`Layout not found for page: ${page.id}`); - return null; +if (!layout) { + console.warn(`Layout not found for page: ${page.id}`); + // Consider showing an error state or placeholder to the user + return ( + <div key={page.id} className={classes.groupAccordionWrapper}> + <div className={classes.errorState}> + {t('ux_editor.layout_not_found', { pageId: page.id })} + </div> + </div> + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/packages/ux-editor/src/containers/DesignView/DesignView.test.tsx(4 hunks)frontend/packages/ux-editor/src/containers/DesignView/DesignView.tsx(2 hunks)frontend/packages/ux-editor/src/containers/FormDesigner.test.tsx(0 hunks)frontend/packages/ux-editor/src/containers/FormDesigner.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/packages/ux-editor/src/containers/FormDesigner.test.tsx
🧰 Additional context used
🧬 Code Definitions (2)
frontend/packages/ux-editor/src/containers/DesignView/DesignView.test.tsx (1)
frontend/packages/ux-editor/src/testing/layoutMock.ts (1)
layout1Mock(134-174)
frontend/packages/ux-editor/src/containers/DesignView/DesignView.tsx (2)
frontend/packages/ux-editor/src/containers/DesignView/PageAccordion/PageAccordion.tsx (1)
PageAccordion(38-103)frontend/packages/ux-editor/src/containers/DesignViewNavigation/DesignViewNavigation.tsx (1)
DesignViewNavigation(8-56)
⏰ 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 (12)
frontend/packages/ux-editor/src/containers/FormDesigner.tsx (1)
169-169: Feature flag implementation simplified.The design view is now rendered unconditionally, removing the previous feature flag check. This simplifies the component and moves the feature flag check to the
DesignViewcomponent itself, which is a good approach for better encapsulation.frontend/packages/ux-editor/src/containers/DesignView/DesignView.test.tsx (7)
27-32: Good addition of feature toggle mocks for tests.The mock implementation for
featureToggleUtilsis well-structured and allows for controlling the feature flag behavior in tests, essential for testing the new conditional rendering logic.
68-68: Properly handling console warnings in tests.Good practice to suppress console warnings during tests to keep the test output clean. Make sure to restore the spy after each test to prevent affecting other tests.
Also applies to: 84-84, 126-127, 133-134
136-148: Good test coverage for DesignViewNavigation rendering.These tests properly verify the conditional rendering of the DesignViewNavigation component based on the feature flag, ensuring that the navigation is only displayed when the feature flag is enabled.
150-156: Proper handling of edge cases in tests.This test correctly verifies that the Accordion is not rendered when there are no pages, which is an important edge case to cover.
158-164: Testing group accordion rendering when feature flag is enabled.This test properly verifies that group accordions are rendered when the feature flag is enabled and groups are available.
166-173: Verifying fallback to page accordions when feature flag is disabled.This test properly checks that even when groups are available, they are not rendered when the feature flag is disabled, and instead the standard page accordions are displayed.
175-182: Comprehensive test coverage for page accordion rendering.This test ensures that the page accordions are rendered correctly when the feature flag is disabled, checking that each page in the order is displayed.
frontend/packages/ux-editor/src/containers/DesignView/DesignView.tsx (4)
11-11: Good imports organization for new functionality.The new imports are properly added for the required components and icons needed for the group accordion feature. Including the feature flag utilities for conditional rendering is a good practice.
Also applies to: 18-18, 23-24
133-152: Transition plan needed for mock data.While the mock data approach works well for development, there should be a clear plan for transitioning to real data from the backend.
Consider adding:
- A TODO comment with a ticket reference for replacing the mock data
- Make sure the mock structure matches the expected API contract
- Add error handling for edge cases (e.g., when no pages are available)
#!/bin/bash # Check if there are existing TODOs for replacing the mock data rg -A 1 "TODO.*replace.*mock" --glob="*.ts*" frontend/packages/ux-editor/
208-209: Good use of descriptive variables for conditional logic.Creating named variables for conditions improves readability and makes the code more maintainable. Both
hasGroupsandisTaskNavigationPageGroupsclearly communicate their purpose.
214-222: Clean conditional rendering implementation.The conditional rendering logic is well-structured and clear. It appropriately checks both the feature flag and the presence of groups before deciding what to render.
No need for refactoring now because the component will be changed after remove test mock and implement changes from backend |
Description
For this issue we worked on frontend part to show the page-groups, we use for now just testing mock to show data until the backend part is done.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Style
Tests
Chores
data-testidattribute to enhance testability of the DesignViewNavigation component.