fix(regulations-admin): Update strings, fix impactname #17041
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several modifications across multiple components related to the regulation editing interface. Key changes include the addition of a new prop 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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17041 +/- ##
==========================================
- Coverage 35.61% 35.60% -0.01%
==========================================
Files 6934 6934
Lines 146857 146863 +6
Branches 41755 41755
==========================================
Hits 52297 52297
- Misses 94560 94566 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 24 Passed, 0 Skipped, 15.56s Total Time |
![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
🧹 Outside diff range and nitpick comments (7)
libs/portals/admin/regulations-admin/src/components/impacts/ImpactDate.tsx (2)
45-58: Consider adding aria-description for better accessibilityWhile the explanatory text provides good context visually, consider linking it to the checkbox using aria-describedby for better screen reader support.
<Box marginBottom={2}> <Checkbox label={t(impactMsgs.specificDateApply)} labelVariant="default" checked={hasCustomDate} onChange={() => setHasCustomDate(!hasCustomDate)} + aria-describedby="date-details" /> </Box> <Box marginBottom={3}> - <Text variant="small" color="dark400"> + <Text variant="small" color="dark400" id="date-details"> {t(impactMsgs.specificDateApplyTextDetails)} </Text> </Box>
Line range hint
24-24: Add explicit return type for better type safetyConsider adding an explicit return type to the component for better type safety and documentation.
-export const ImpactDate = (props: ImpactDateProps) => { +export const ImpactDate = (props: ImpactDateProps): JSX.Element => {libs/portals/admin/regulations-admin/src/components/impacts/ReferenceText.tsx (1)
Line range hint
14-18: Enhance TypeScript type definitionsWhile the component follows TypeScript best practices, consider these improvements:
- Add JSDoc documentation for the
asBaseprop to explain its purpose- Explicitly type the
commentsfield in the component's usage ofRegulationtypetype ReferenceTextProps = { baseName: RegName regulation: Regulation + /** When true, displays the component in base regulation mode */ asBase?: boolean }libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (3)
Line range hint
282-321: Enhance error handling in mutation responsesThe error handling in the mutations could be improved. Currently, it only checks for multiple errors (
res.errors.length > 1), which might miss single error cases.Consider updating the error handling logic:
- if (res.errors && res.errors.length > 1) { + if (res.errors?.length > 0) { throw res.errors[0] }
Line range hint
424-433: Consider enhancing validation logicThe current validation checks for presence but could benefit from additional validation rules:
- Length limits for title and text
- Format validation for specific fields
- Type checking for critical fields
Consider adding more comprehensive validation:
const isValidImpact = () => { + const MAX_TITLE_LENGTH = 255; return ( activeChange.date?.value && - activeChange.title?.value && + activeChange.title?.value && + activeChange.title.value.length <= MAX_TITLE_LENGTH && activeChange.text?.value && activeChange.appendixes.every( (apx) => apx.title?.value && apx.text?.value, ) ) }
Line range hint
1-50: Consider improving component reusabilityAs this component resides in a library folder, it should be designed for reuse across different NextJS apps. Consider:
- Extracting regulation-specific logic into custom hooks
- Making the component more generic by accepting customizable validation and transformation functions
Example approach:
// Custom hook for regulation logic const useRegulationChange = (draft, change) => { // Move regulation-specific logic here return { activeChange, setActiveChange, isValid, save, // ... other regulation-specific logic } } // More generic component const EditChange = ({ draft, change, validate = defaultValidate, transform = defaultTransform, ...props }) => { const { activeChange, setActiveChange, isValid, save } = useRegulationChange(draft, change) // ... rest of the component }libs/portals/admin/regulations-admin/src/lib/messages.ts (1)
428-432: LGTM! Consider adding a description field for consistency.The new message descriptor follows the established patterns and provides clear guidance. The implementation is type-safe and maintains the file's structure.
Consider adding an optional
descriptionfield to provide additional context for translators, following the pattern used in other complex messages in this file:specificDateApplyTextDetails: { id: 'ap.regulations-admin:change-applied-on-specific-date-text-details', defaultMessage: 'Sjálfgefið er að breytingar taki gildi þegar í stað, daginn eftir útgáfudag. Veljið dagsetningu úr dagatali ef gildistaka er á ákveðinni dagsetningu.', + description: 'Explanatory text shown when selecting a specific effective date for regulation changes', },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx(1 hunks)libs/portals/admin/regulations-admin/src/components/impacts/ImpactDate.tsx(3 hunks)libs/portals/admin/regulations-admin/src/components/impacts/ReferenceText.tsx(1 hunks)libs/portals/admin/regulations-admin/src/lib/messages.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/regulations-admin/src/components/impacts/ImpactDate.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/regulations-admin/src/components/impacts/ReferenceText.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/regulations-admin/src/lib/messages.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (4)
libs/portals/admin/regulations-admin/src/components/impacts/ImpactDate.tsx (2)
1-7: LGTM! Imports follow best practices
The imports are well-organized and maintain effective tree-shaking by importing specific components from the UI library.
80-83: LGTM! Improved state management
The button handler now properly resets both the date value and the custom date flag, ensuring consistent state management.
libs/portals/admin/regulations-admin/src/components/impacts/ReferenceText.tsx (1)
29-30: LGTM! Text constant updates are consistent
The terminology changes align well with the mapping function mapRegulationTypeToDisplayString and maintain consistent string templating.
libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (1)
449-449: LGTM: Clear and meaningful prop addition
The new asBase prop effectively distinguishes between base and non-base regulation types.
kodiakhq
bot
deleted the
regulations-admin/strings-date-and-impactname
branch
* Fix strings date and impact name * Radio buttons --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Update strings, fix impactname
Why
Bugfixes 🐛
and
Improvement 🧹
Checklist:
Summary by CodeRabbit
New Features
asBasein theReferenceTextcomponent for enhanced draft type rendering.Improvements
EditChangeandImpactDatecomponents for better user experience.ReferenceTextcomponent to clarify terminology between amending and base regulations.User Interface
ImpactDatecomponent.