feat(ui): cleaner look for applied controls and audit forms #1456
Conversation
WalkthroughThe pull request introduces localization updates and a significant UI restructuring in the frontend. Two language files ( Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🪧 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 (
|
![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 (2)
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte (2)
47-53: Ensure consistent naming between the label and field properties.
TheScorecomponent referencesprogress_fieldwhile the label ism.progress(). If you want uniformity, consider renaming the field to “progress” for better readability and consistency.
64-153: Consider extracting repetitive form elements into a reusable sub-component.
A large set of similar fields (TextField, Select, etc.) is nested in theDropdown. To simplify maintenance and reduce block size, you could factor out these blocks into a new form sub-component and pass needed props (e.g.,form,cacheLocks, etc.). This also improves testability and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: migrations-check (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
frontend/src/lib/components/Forms/ModelForm/AppliedControlPolicyForm.svelte (1)
7-7: Good addition for the Dropdown import.
No issues spotted with the import statement; it cleanly introduces the newDropdowncomponent into this file.frontend/messages/en.json (1)
1147-1148: Localization entry and text alignment look good.
The newly added"sectionMoved"and"more"keys are consistent with existing localization patterns. No further adjustments needed.frontend/messages/fr.json (1)
1146-1147: Newly added French localization is correctly aligned.
Both"sectionMoved": "Section déplacée ici"and"more": "Plus"follow expected translation patterns. Everything looks good.
ab-smith
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.svelte (3)
110-110: Enhance accessibility and styling of the Dropdown.Consider these improvements:
- Add an aria-label for better accessibility
- Define a base text color along with the hover state
-<Dropdown open={false} style="hover:text-primary-700" icon="fa-solid fa-list" header={m.more()}> +<Dropdown + open={false} + style="text-gray-700 hover:text-primary-700" + icon="fa-solid fa-list" + header={m.more()} + aria-label={m.more()}>
111-120: Add error handling for suggestions state.The conditional rendering depends on the
suggestionsstate which is set in the framework change handler. Consider adding error handling and a loading state.-{#if context === 'create' && suggestions} +{#if context === 'create' && suggestions !== undefined} <Checkbox {form} field="create_applied_controls_from_suggestions" label={m.suggestControls()} helpText={m.createAppliedControlsFromSuggestionsHelpText()} cacheLock={cacheLocks['create_applied_controls_from_suggestions']} bind:cachedValue={formDataCache['create_applied_controls_from_suggestions']} /> +{:else if context === 'create'} + <div class="text-gray-500">{m.loadingSuggestions()}</div> {/if}
129-175: Consider grouping related fields within the Dropdown.The Dropdown contains many fields which might affect usability. Consider grouping related fields using semantic HTML or additional nested components for better organization.
+<div class="space-y-4"> + <div class="space-y-2"> + <h3 class="text-sm font-medium text-gray-700">{m.identification()}</h3> <TextField {form} field="ref_id" label={m.refId()} cacheLock={cacheLocks['ref_id']} bind:cachedValue={formDataCache['ref_id']} /> <TextField {form} field="version" label={m.version()} cacheLock={cacheLocks['version']} bind:cachedValue={formDataCache['version']} /> + </div> + <div class="space-y-2"> + <h3 class="text-sm font-medium text-gray-700">{m.progress()}</h3> <Select {form} options={model.selectOptions['status']} field="status" label={m.status()} cacheLock={cacheLocks['status']} bind:cachedValue={formDataCache['status']} /> <AutocompleteSelect {form} multiple options={getOptions({ objects: model.foreignKeys['reviewers'], label: 'email' })} field="reviewers" cacheLock={cacheLocks['reviewers']} bind:cachedValue={formDataCache['reviewers']} label={m.reviewers()} /> <TextField type="date" {form} field="due_date" label={m.dueDate()} helpText={m.dueDateHelpText()} cacheLock={cacheLocks['due_date']} bind:cachedValue={formDataCache['due_date']} /> + </div> + <div class="space-y-2"> + <h3 class="text-sm font-medium text-gray-700">{m.notes()}</h3> <TextArea {form} field="observation" label={m.observation()} cacheLock={cacheLocks['observation']} bind:cachedValue={formDataCache['observation']} /> + </div> +</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.svelte(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
frontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.svelte (2)
12-12: LGTM! Clean import addition.The Dropdown component import follows the established pattern and uses the correct path alias.
65-65: Verify translation key exists.The label has been changed from
m.framework()tom.targetFramework(). Let's verify this translation key exists in both language files.✅ Verification successful
Translation key verified successfully
The
targetFrameworktranslation key exists and is properly localized across all language files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the targetFramework translation key exists in both language files echo "Checking for targetFramework in language files..." rg "targetFramework" frontend/src/lib/i18n/Length of output: 215
Script:
#!/bin/bash # First locate i18n files echo "Locating i18n files..." fd -e json "en|de" --full-path echo -e "\nChecking for targetFramework in found files..." fd -e json "en|de" --full-path --exec rg "targetFramework" {}Length of output: 1449
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.svelte (2)
113-113: Consider UX implications of hiding form fields.While the dropdown improves UI organization, hiding form fields by default (
open={false}) might impact user experience if required fields are not immediately visible.Consider one of these approaches:
- Set
open={true}by default if the dropdown contains required fields- Add visual indicators (e.g., asterisk) to the dropdown header when it contains required fields
- Move required fields outside the dropdown
132-178: Consider grouping related form fields.The form fields in the dropdown appear to be in a somewhat arbitrary order. Consider grouping related fields together for better organization:
Suggested grouping:
- Identification fields (ref_id, version)
- Status and timeline fields (status, due_date)
- People fields (reviewers)
- Content fields (observation)
<Dropdown open={false} style="hover:text-primary-700" icon="fa-solid fa-list" header={m.more()}> <!-- Checkboxes remain at top --> <!-- Group 1: Identification --> <TextField {form} field="ref_id" label={m.refId()} ... /> <TextField {form} field="version" label={m.version()} ... /> <!-- Group 2: Status and Timeline --> <Select {form} field="status" label={m.status()} ... /> <TextField type="date" {form} field="due_date" label={m.dueDate()} ... /> <!-- Group 3: People --> <AutocompleteSelect {form} field="reviewers" label={m.reviewers()} ... /> <!-- Group 4: Content --> <TextArea {form} field="observation" label={m.observation()} ... /> </Dropdown>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.svelte(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
frontend/src/lib/components/Forms/ModelForm/ComplianceAssessmentForm.svelte (3)
12-12: LGTM!The import of the Dropdown component aligns with the PR's objective of providing a cleaner UI structure.
114-123: LGTM! Well-structured conditional rendering.The conditional rendering of the suggestions checkbox based on context and suggestions state is implemented correctly.
79-79: Verify translation key exists.The label has been changed to use
m.targetFramework(). Please ensure this translation key exists in both language files.✅ Verification successful
Translation key verified successfully
The
targetFrameworktranslation key is properly defined in all language files with appropriate content (e.g., "Target framework" in English).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the targetFramework translation key exists in language files # Search for targetFramework in language files echo "Checking en.json..." rg '"targetFramework"' frontend/src/lib/i18n/en.json -A 1 echo "Checking fr.json..." rg '"targetFramework"' frontend/src/lib/i18n/fr.json -A 1Length of output: 357
Script:
#!/bin/bash # First, find all JSON files that might contain translations echo "Searching for translation files..." fd -e json -x grep -l "framework\|translation" {} \; # Then search for targetFramework in those files echo -e "\nSearching for targetFramework in JSON files..." fd -e json -x grep -l "targetFramework" {} \;Length of output: 1171
Script:
#!/bin/bash # Check the content of targetFramework in English translation file echo "Content of targetFramework in English translation:" rg '"targetFramework"' frontend/messages/en.json -A 1 -C 1Length of output: 276
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/tests/utils/test-data.ts (1)
252-253: Add a comment explaining why these properties are commented out.The properties
categoryandcsf_functionhave been commented out in bothreferenceControlandreferenceControl2objects. While this aligns with the UI improvements mentioned in the PR objectives, it would be helpful to add a comment explaining why these fields are being disabled and if/when they might be needed again.referenceControl: { name: 'POL.PHYSICAL - Physical security policy', + // TODO: category and csf_function fields are temporarily disabled as part of the UI cleanup // category: 'policy', // csf_function: 'govern',Also applies to: 263-264
frontend/tests/utils/test-utils.ts (1)
225-232: Document the UI cleanup effort.Multiple form fields have been commented out as part of the UI cleanup. Consider adding a comment block explaining the rationale behind this UI restructuring and listing all the affected fields for better maintainability.
appliedControlsPage: async ({ page }, use) => { const sPage = new PageContent(page, '/applied-controls', 'Applied controls', [ + // The following fields have been temporarily hidden as part of the UI cleanup effort + // to provide a cleaner look for applied controls forms (PR #1456) { name: 'name', type: type.TEXT }, { name: 'description', type: type.TEXT },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/tests/utils/test-data.ts(2 hunks)frontend/tests/utils/test-utils.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
frontend/tests/utils/test-utils.ts (1)
213-214: LGTM! Consistent with test-data.ts changes.The commented-out fields in
referenceControlsPagealign with the changes in test-data.ts, showing good coordination in the test suite updates.
Summary by CodeRabbit
Release Notes
New Features
User Interface
Bug Fixes