fix(ojoi): Bug fixes and add adverts to web project. #16388
Conversation
WalkthroughThe pull request introduces several changes across multiple files related to the Official Journal of Iceland application. A new React component, 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 #16388 +/- ##
==========================================
- Coverage 36.75% 36.75% -0.01%
==========================================
Files 6807 6808 +1
Lines 140978 140991 +13
Branches 40149 40156 +7
==========================================
- Hits 51818 51816 -2
- Misses 89160 89175 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review 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: 3
🧹 Outside diff range and nitpick comments (10)
libs/application/templates/official-journal-of-iceland/src/lib/constants.ts (2)
7-7: Consider adding a comment explaining the purpose ofOJOI_DF.The new constant
OJOI_DFhas been added with the value'd. MMMM yyyy.'. While the format suggests it's a date format string, it would be helpful to add a brief comment explaining its specific purpose and usage within the Official Journal of Iceland application.Consider adding a comment above the constant, like this:
// Date format string for displaying dates in the Official Journal of Iceland (e.g., "1. January 2023.") export const OJOI_DF = 'd. MMMM yyyy.'This addition aligns with the coding guidelines for
libs/**/*files by improving code readability and maintainability across different NextJS apps.
Line range hint
1-83: Consider improving file organization and documentation.The file adheres to the coding guidelines for
libs/**/*files by using TypeScript and defining reusable constants and types. However, consider the following improvements to enhance maintainability and readability:
- Group related constants and enums together, separating them with comments explaining their purpose.
- Add brief comments for non-obvious constants or enums to explain their usage.
- Consider splitting this file into smaller, more focused files if it grows larger (e.g., separate files for date-related constants, file-related constants, etc.).
Here's an example of how you could start reorganizing the file:
// Date and time constants export const OJOI_DF = 'd. MMMM yyyy.' export const INTERVAL_TIMER = 3000 export const DEBOUNCE_INPUT_TIMER = 333 // File-related constants export const ALLOWED_FILE_TYPES = ['.pdf', '.doc', '.docx'] export const FILE_SIZE_LIMIT = 10000000 // Regular expressions export const emailRegex = /^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}$/i // Enums export enum AnswerOption { YES = 'yes', NO = 'no', } export enum ApplicationAttachmentType { ORIGINAL = 'frumrit', ADDITIONS = 'fylgiskjol', } // ... (continue grouping and commenting)These changes will improve the overall structure and make the file easier to maintain as it grows.
apps/web/components/OfficialJournalOfIceland/OJOISearchListView.tsx (3)
13-13: Prop type change looks good, consider using nullish coalescing for default value.The change to make the
advertsprop optional is a good improvement, allowing the component to handle cases where advertisements might not be available. This change enhances the component's flexibility and adheres to TypeScript best practices.Consider providing a default empty array using nullish coalescing to simplify the usage within the component:
adverts?: OfficialJournalOfIcelandAdvertsResponse['adverts'] = []This would allow you to remove the optional chaining operator (
?.) when mapping overadverts, making the code slightly cleaner.
30-30: Usage of optional chaining is correct, consider memoization for performance.The use of optional chaining (
?.) when mapping overadvertsis correct and consistent with the new prop type. This ensures that the component doesn't throw an error ifadvertsis undefined.For potential performance optimization, consider memoizing the mapped JSX if
advertsis expected to be a large array:const memoizedAdverts = useMemo(() => adverts?.map((ad) => ( // ... existing JSX )), [adverts] ); // Then in your JSX: {memoizedAdverts}This would prevent unnecessary re-renders if the component re-renders for reasons unrelated to the
advertsprop.
Line range hint
1-68: Overall structure adheres to NextJS and React best practices.The component structure is well-organized and follows React and NextJS best practices. It's a functional component with proper use of TypeScript for type safety. The single responsibility of rendering a table of advertisements is maintained, which is good for modularity and maintainability.
For consistency and to follow modern React practices, consider using the
FCtype for the component definition:import { FC } from 'react'; type OJOISearchListViewProps = { adverts?: OfficialJournalOfIcelandAdvertsResponse['adverts']; locale: Locale; }; export const OJOISearchListView: FC<OJOISearchListViewProps> = ({ adverts, locale, }) => { // ... component body };This explicitly defines the component as a functional component and can help with type inference in some cases.
libs/application/templates/official-journal-of-iceland/src/lib/messages/publishing.ts (1)
24-25: LGTM! Consider adding a minor clarification.The update to the
communicationIntromessage is appropriate and provides clear instructions in Icelandic, which is more suitable for the Official Journal of Iceland application. The change maintains the existing structure and doesn't affect reusability, TypeScript usage, or tree-shaking practices.Consider adding a brief clarification about the purpose of providing multiple contact methods. For example:
- 'Hér getur þú skráð inn tölvupóstfang og/eða símanúmer þess sem best er að hafa samskipti við vegna málsins, hægt að skrá fleiri en einn.', + 'Hér getur þú skráð inn tölvupóstfang og/eða símanúmer þess sem best er að hafa samskipti við vegna málsins. Hægt er að skrá fleiri en einn tengilið til að tryggja góð samskipti.',This addition clarifies the purpose of allowing multiple entries and maintains the helpful tone of the message.
libs/application/templates/official-journal-of-iceland/src/components/signatures/Chairman.tsx (3)
93-100: LGTM! Consider updating the component'snameprop for consistency.The changes to this
SignatureMembercomponent look good. The props have been correctly updated to use the "name" field instead of the "after" field.For consistency, consider updating the
nameprop to remove the typo in "comittee":-name={`signature.comittee.member.name`} +name={`signature.committee.member.name`}
105-112: LGTM! Consider updating the component'snameprop for consistency.The changes to this
SignatureMembercomponent look good. The props have been correctly updated to use the "after" field instead of the "name" field.For consistency, consider updating the
nameprop to remove the typo in "comittee":-name={`signature.comittee.member.after`} +name={`signature.committee.member.after`}
Line range hint
1-128: Overall, the changes look good. Consider a global rename for consistency.The modifications to the
Chairmancomponent are correct and maintain its reusability across different NextJS apps. The TypeScript usage for props is preserved, and there are no apparent issues that would affect tree-shaking or bundling.To improve consistency throughout the file, consider performing a global rename to correct the spelling of "committee":
sed -i 's/comittee/committee/g' libs/application/templates/official-journal-of-iceland/src/components/signatures/Chairman.tsxThis will ensure that all instances of "comittee" are replaced with the correct spelling "committee" throughout the file.
apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx (1)
84-92: Evaluate the use of'fetchPolicy: no-cache'The
fetchPolicyis set to'no-cache'for each query, which disables caching and forces the application to fetch fresh data every time. This may impact performance due to increased network requests. Consider whether this is necessary or if the default caching behavior can be utilized to improve performance.Also applies to: 104-112, 124-132
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
- apps/web/components/OfficialJournalOfIceland/OJOIAdvertCards.tsx (1 hunks)
- apps/web/components/OfficialJournalOfIceland/OJOISearchListView.tsx (2 hunks)
- apps/web/components/OfficialJournalOfIceland/index.ts (1 hunks)
- apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx (4 hunks)
- apps/web/screens/OfficialJournalOfIceland/messages.ts (1 hunks)
- libs/application/templates/official-journal-of-iceland/src/components/signatures/Chairman.tsx (1 hunks)
- libs/application/templates/official-journal-of-iceland/src/components/signatures/RegularMember.tsx (2 hunks)
- libs/application/templates/official-journal-of-iceland/src/lib/constants.ts (1 hunks)
- libs/application/templates/official-journal-of-iceland/src/lib/messages/attachments.ts (3 hunks)
- libs/application/templates/official-journal-of-iceland/src/lib/messages/publishing.ts (1 hunks)
- libs/application/templates/official-journal-of-iceland/src/lib/utils.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
apps/web/components/OfficialJournalOfIceland/OJOIAdvertCards.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/components/OfficialJournalOfIceland/OJOISearchListView.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/components/OfficialJournalOfIceland/index.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/OfficialJournalOfIceland/messages.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/application/templates/official-journal-of-iceland/src/components/signatures/Chairman.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/application/templates/official-journal-of-iceland/src/components/signatures/RegularMember.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/application/templates/official-journal-of-iceland/src/lib/constants.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."
libs/application/templates/official-journal-of-iceland/src/lib/messages/attachments.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."
libs/application/templates/official-journal-of-iceland/src/lib/messages/publishing.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."
libs/application/templates/official-journal-of-iceland/src/lib/utils.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 (16)
apps/web/components/OfficialJournalOfIceland/OJOIAdvertCards.tsx (3)
1-6: LGTM: Imports are well-organized and follow best practices.The imports are correctly structured, using absolute imports as per NextJS best practices. They provide necessary types, components, and utilities for the implementation, enhancing type safety and maintainability.
8-11: LGTM: Props type is well-defined and follows TypeScript best practices.The Props type is correctly defined, making good use of TypeScript features:
- It leverages types from the API schema and shared types, ensuring consistency.
- The
advertsprop is optional, providing flexibility in component usage.- The
localeprop is required, ensuring necessary information is always provided.
1-30: Overall, excellent implementation of the OJOIAdvertCards component.This new component is well-structured, type-safe, and adheres to NextJS and React best practices. It efficiently renders advertisement cards using appropriate UI components and hooks. The code is clean, readable, and maintainable.
Key strengths:
- Proper use of TypeScript for enhanced type safety.
- Adherence to NextJS file structure and import practices.
- Efficient use of existing components and hooks.
- Clear and concise implementation of the component logic.
With the minor optimization suggestion implemented, this component will be a solid addition to the project.
libs/application/templates/official-journal-of-iceland/src/lib/messages/attachments.ts (4)
7-7: LGTM: Consistent terminology updateThe change from "Viðaukar og fylgirit" to "Viðaukar og fylgiskjöl" appears to be a consistent terminology update in Icelandic. This modification aligns with the overall changes in the file and doesn't affect the code structure or reusability.
18-18: LGTM: Consistent terminology updateThe change from "Viðaukar og fylgirit" to "Viðaukar og fylgiskjöl" is consistent with the previous update in line 7. This ensures uniformity in terminology throughout the messages object.
58-58: LGTM: Consistent terminology updateThe change from "Fylgirit (I, II, III..)" to "Fylgiskjöl (I, II, III..)" aligns with the previous terminology updates in this file. This modification ensures consistency across all related messages.
Line range hint
1-62: Overall assessment: Changes adhere to coding guidelinesThe modifications in this file consist of consistent terminology updates in Icelandic messages. These changes:
- Maintain the reusability of the messages across different NextJS apps.
- Preserve the existing TypeScript usage for defining props and exporting types.
- Do not affect tree-shaking or bundling practices.
The updates are well-structured and align with the coding guidelines for files in the
libs/**/*pattern.libs/application/templates/official-journal-of-iceland/src/components/signatures/RegularMember.tsx (1)
121-123: Clarify the purpose of swapping "name" and "after" fieldsThe changes in the
SignatureMembercomponents swap the roles of the "name" and "after" fields. While the implementation looks correct, there are some concerns:
- This swap might introduce inconsistencies in data handling if not applied uniformly across the application.
- The purpose of this change is not immediately clear. Could you provide more context on why this swap is necessary or beneficial?
- These changes might affect data persistence and retrieval. Have corresponding changes been made in the backend to handle this new structure?
To ensure consistency, let's check for other occurrences of these field names:
Please clarify the rationale behind these changes and confirm that they have been consistently applied throughout the codebase.
Also applies to: 128-128, 138-140, 145-145
✅ Verification successful
Verification Successful: Changes Are Localized
The modifications to the
nameandafterfields in theRegularMember.tsxcomponent are confined to this file only. No other parts of the codebase reference these fields, ensuring that the changes do not introduce inconsistencies elsewhere.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of "signature.regular.member.name" and "signature.regular.member.after" rg --type typescript "signature\.regular\.member\.(name|after)"Length of output: 100
Script:
#!/bin/bash # Search for other occurrences of "signature.regular.member.name" and "signature.regular.member.after" in TypeScript files rg "signature\.regular\.member\.(name|after)" --glob "*.ts" --glob "*.tsx"Length of output: 439
apps/web/screens/OfficialJournalOfIceland/messages.ts (5)
38-41: LGTM: New message for latest adverts added correctly.The new
latestAdvertsmessage follows the existing pattern and naming conventions. The ID is consistent with other messages in thehomesection, and the default message is appropriately in Icelandic.
49-52: LGTM: New message for empty search result added correctly.The new
emptySearchResultmessage adheres to the existing pattern and naming conventions. The ID is consistent with other messages in thesearchsection, and the default message is appropriately in Icelandic.
53-56: LGTM: New message for error fetching adverts title added correctly.The new
errorFetchingAdvertsTitlemessage follows the existing pattern and naming conventions. The ID is consistent with other messages in thesearchsection, and the default message is appropriately in Icelandic.
57-61: LGTM: New message for error fetching adverts message added correctly.The new
errorFetchingAdvertsMessagemessage adheres to the existing pattern and naming conventions. The ID is consistent with other messages in thesearchsection, and the default message is appropriately in Icelandic. The multi-line format improves readability for longer messages.
38-61: Summary: New messages align with PR objectives and enhance localization.The added messages (
latestAdverts,emptySearchResult,errorFetchingAdvertsTitle, anderrorFetchingAdvertsMessage) are well-integrated into the existing file structure and follow consistent naming conventions. These additions enhance the localization support for advertisements and search functionality, aligning with the PR objective of adding advertisements to the web project.The new messages provide appropriate text for:
- Displaying latest advertisements
- Handling empty search results
- Showing error messages when fetching advertisements fails
These changes will improve the user experience by providing localized feedback for various scenarios related to advertisements.
libs/application/templates/official-journal-of-iceland/src/lib/utils.ts (3)
16-16: LGTM: New import adheres to coding guidelines.The addition of the
OJOI_DFimport from./constantsis consistent with the coding guidelines for thelibsdirectory. It uses named imports, which is beneficial for tree-shaking and bundling practices.
214-215: LGTM: Improved date formatting using constant.The update to use
OJOI_DFfor date formatting enhances flexibility and maintainability. It adheres to the coding guidelines for thelibsdirectory by improving reusability across different NextJS apps. The preservation of theislocale maintains proper internationalization support.
Line range hint
1-324: Overall assessment: Changes adhere to coding guidelines and improve code quality.The modifications in this file, particularly the new import and the updated date formatting in
signatureTemplate, align well with the coding guidelines for thelibsdirectory. These changes enhance reusability and maintainability while preserving TypeScript usage and effective tree-shaking practices. The code remains consistent with the existing structure and doesn't introduce any apparent issues.
Datadog ReportAll test runs ✅ 15 Total Test Services: 0 Failed, 15 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
jonbjarnio
added
the
deprecated:automerge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/web/components/OfficialJournalOfIceland/OJOIAdvertCard.tsx (1)
62-70: Approved with a minor suggestion for improvementThe changes to the
Boxcomponents improve the layout flexibility and spacing consistency for the categories. The addition offlexWrap="wrap"allows for better responsiveness, especially when there are many categories.To further improve the code, consider implementing the optional chain suggestion:
- {categories && categories.length && ( + {categories?.length && (This change makes the code more concise and maintains the same functionality.
🧰 Tools
🪛 Biome
[error] 69-69: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx (1)
173-202: LGTM: Latest adverts section well-implemented with a minor suggestionThe new section for latest adverts is well-implemented, handling loading states, errors, and conditional rendering of adverts appropriately. The use of SkeletonLoader for loading states and localized error messages are good practices.
Suggestion for improvement:
Consider adding anaria-liveattribute to the Stack component containing the adverts to improve accessibility for screen readers. This will announce changes in the adverts content to users relying on assistive technologies.Example:
- <Stack space={3}> + <Stack space={3} aria-live="polite">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/web/components/OfficialJournalOfIceland/OJOIAdvertCard.tsx (1 hunks)
- apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx (4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/web/components/OfficialJournalOfIceland/OJOIAdvertCard.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🪛 Biome
apps/web/components/OfficialJournalOfIceland/OJOIAdvertCard.tsx
[error] 69-69: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
apps/web/screens/OfficialJournalOfIceland/OJOIHome.tsx (4)
Line range hint
3-27: LGTM: New imports added for adverts functionalityThe new imports, including
useQueryfrom Apollo Client and additional types, are correctly added to support the new adverts functionality.
35-49: LGTM: New component and query imports addedThe new imports for
OJOIAdvertCard,OJOIAdvertCards, and theADVERTS_QUERYare correctly added to support the new adverts functionality.
155-172: LGTM: Main categories section addedThe new section for main categories is well-implemented, using the project's UI components and following responsive design principles with GridRow and GridColumn. The addition of a title and a link to view all categories enhances user navigation.
75-91: Consider the impact of 'no-cache' fetch policyThe implementation of the
useQueryhook to fetch adverts is correct and follows Apollo Client best practices. However, the 'no-cache' fetch policy will result in a network request on every render. While this ensures fresh data, it may impact performance, especially for frequently re-rendered components.Consider if caching would be beneficial for your use case. If real-time data is not critical, you might want to use a different fetch policy like 'cache-first' or 'cache-and-network' to improve performance.
To verify the impact of the 'no-cache' policy, you can run the following command to check for multiple identical network requests:
This will help identify if there are multiple instances of this query that might benefit from caching.
✅ Verification successful
Verified: Single usage of 'ADVERTS_QUERY'
The search revealed only one instance of
useQuerywithADVERTS_QUERYin the codebase. Therefore, the 'no-cache' fetch policy will not lead to multiple unnecessary network requests. The current implementation is appropriate if fresh data is required for this single query.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for network requests related to advert fetching rg --type typescript "useQuery.*ADVERTS_QUERY" -A 10 -B 5Length of output: 94
Script:
#!/bin/bash # Search for network requests related to advert fetching in TypeScript files using file extensions rg "useQuery.*ADVERTS_QUERY" -g "*.ts" -g "*.tsx" -A 10 -B 5Length of output: 1643
What
Bugfixes and advert on web frontpage
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Localization
Improvements