Platform Banner extended to show terms and privacy #7092
Conversation
WalkthroughThe changes in this pull request primarily involve updates to the 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
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (3)
10-10: LGTM: Constant is well-defined, but consider adding a comment for clarity.The
URL_NOT_CLICKABLE_MARKERconstant is correctly defined and follows proper naming conventions. To enhance code readability, consider adding a brief comment explaining its purpose and usage.// Marker used to identify non-clickable release note URLs const URL_NOT_CLICKABLE_MARKER = 'NOT_CLICKABLE';
14-15: LGTM: Good use of custom hook, but consider adding type annotation.The
useConfighook is correctly used to retrieve thelocationsobject. To improve type safety and code readability, consider adding a type annotation for thelocationsobject.const { locations }: { locations: { terms?: string; privacy?: string } } = useConfig();This assumes that
termsandprivacyare optional string properties oflocations. Adjust the type according to the actual structure of thelocationsobject.
31-31: LGTM: Good use of Trans component with new elements.The addition of the icon and RouterLink components enhances the banner's functionality and appearance. The use of optional chaining for
locations.termsandlocations.privacyis a good practice.Consider removing the
underline="always"prop from the RouterLink components if this style is or can be handled by global link styles. This would improve consistency across the application:terms: <RouterLink to={locations?.terms ?? ''} />, privacy: <RouterLink to={locations?.privacy ?? ''} />,Also, ensure that the URLs for terms and privacy are properly sanitized to prevent potential XSS attacks, especially if they come from an external source.
Also applies to: 34-35
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/core/i18n/en/translation.en.json (1 hunks)
- src/core/ui/content/DashboardBanner.tsx (1 hunks)
- src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.md (1 hunks)
- src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/core/i18n/en/translation.en.json (1)
Pattern
src/**/*.json: Review the JSON files for correct syntax and structure.
Ensure that the configuration and data are accurate and follow the project's standards.
Check for sensitive data exposure and ensure that the data is properly validated.src/core/ui/content/DashboardBanner.tsx (1)
Pattern
src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.md (1)
Pattern
**/*.md: Ensure the documentation is clear, concise, and up-to-date, with accurate instructions.src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (1)
Pattern
src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
🪛 LanguageTool
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.md
[uncategorized] ~9-~9: A comma is probably missing here.
Context: ...ting to the URL value; ##### Regarding formatting you can use the following tags: - `<sm...(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
🪛 Markdownlint
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.md
3-3: Expected: h4; Actual: h5
Heading levels should only increment by one level at a time(MD001, heading-increment)
3-3: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
9-9: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
🔇 Additional comments (4)
src/core/ui/content/DashboardBanner.tsx (2)
5-5: LGTM: Import statement updated correctly.The addition of
Boxto the import statement is consistent with its usage in the component.
11-11: LGTM: Interface updated correctly.The addition of the
isLinkproperty to theDashboardBannerPropsinterface is correctly typed and consistent with the component's updated behavior.src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (1)
2-2: LGTM: New imports are correctly added and follow best practices.The new imports enhance the component's functionality by adding an icon and routing capabilities. The use of the custom
useConfighook suggests good separation of concerns.Also applies to: 7-8
src/core/i18n/en/translation.en.json (1)
36-37: LGTM! Consider these points for implementation.The changes to the "releaseNotes" section look good and align with the PR objectives. However, please consider the following:
- Ensure that the component rendering this text can properly handle the HTML-like tags (
<small>,<icon/>,<terms>,<privacy>).- The
NOT_CLICKABLEparameter in the URL is unconventional. Make sure the component using this translation has the necessary logic to handle this parameter correctly.To verify the proper handling of these changes, you can run the following script:
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (3)
10-10: LGTM: Constant for non-clickable marker added.The
URL_NOT_CLICKABLE_MARKERconstant is correctly implemented to support non-clickable updates as per the PR objectives.Consider moving this constant to a separate constants file if it might be used elsewhere in the application for better maintainability.
20-21: LGTM with suggestion: Logic for non-clickable URLs implemented.The
notClickablevariable correctly implements the logic for determining if the release notes URL is clickable, as per the PR objectives.Consider improving type safety and error handling:
const notClickable = typeof releaseNotesUrl === 'string' && releaseNotesUrl.includes(URL_NOT_CLICKABLE_MARKER);This change ensures that
releaseNotesUrlis a string before callingincludes(), preventing potential runtime errors.
27-35: LGTM with suggestion: Banner and Trans component updated correctly.The changes to the DashboardBanner and Trans components correctly implement the new features as per the PR objectives. The isLink prop and new terms and privacy links are appropriately added.
Consider improving the null handling for the locations URLs:
terms: <RouterLink to={locations?.terms || '#'} underline="always" />, privacy: <RouterLink to={locations?.privacy || '#'} underline="always" />,This change ensures that a valid URL is always provided, even if the locations object is undefined or the specific URL is missing.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (1)
Pattern
src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
🔇 Additional comments (2)
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (2)
2-2: LGTM: New imports are correctly added and utilized.The new imports (CampaignOutlinedIcon, RouterLink, and useConfig) are appropriately added and used within the component, enhancing its functionality as per the PR objectives.
Also applies to: 7-8
14-14: LGTM: Config hook used to retrieve locations.The
useConfighook is correctly utilized to retrieve thelocationsobject, which is essential for implementing the terms and privacy links as specified in the PR objectives.
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.md (3)
1-2: Improve document structure with proper heading hierarchy.The document should start with a level 1 heading for better navigation and structure.
Apply this diff:
-### ReleaseNotesBanner +# ReleaseNotesBanner
3-8: Improve visibility logic section structure and clarity.The section about banner visibility could be better structured for clarity.
Apply these changes:
-##### The banner visibility logic depends on: +## Banner Visibility Logic -1. platform.latestReleaseDiscussion from LatestReleaseDiscussion query; -2. the `releaseNotes.url` value from the translations (it's added to the local storage once closed, so make sure it's unique); - 1. note that you can control the banner wrapper to be static (not clickable) by setting the `releaseNotes.isClickable` value to "FALSE" and to "TRUE" if you want to be clickable (navigating to the url value); The `releaseNotes.isClickable: "FALSE"` is useful if you're updating the users with terms and privacy (internal links); +The banner's visibility is controlled by two main factors: + +1. The `platform.latestReleaseDiscussion` from LatestReleaseDiscussion query +2. The `releaseNotes.url` value from translations (stored in local storage once closed, ensure uniqueness) + +### Banner Interaction + +You can control the banner's clickability using `releaseNotes.isClickable`: +- Set to "FALSE" for a static, non-clickable banner +- Set to "TRUE" for a clickable banner that navigates to the specified URL + +Note: Static banners (isClickable: "FALSE") are particularly useful for terms and privacy updates with internal links.🧰 Tools
🪛 Markdownlint
3-3: Expected: h4; Actual: h5
Heading levels should only increment by one level at a time(MD001, heading-increment)
3-3: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
9-15: Enhance formatting section with better organization and details.The formatting section could be improved with better organization and more detailed information about the icon component.
Apply these changes:
-##### Regarding formatting you can use the following tags: +## Available Formatting Tags -The following HTML-like tags are supported: +The following tags are supported for content formatting: -- `<small></small>` -- `<big></big>` -- `<terms></terms>` -- `<privacy></privacy>` -- `<icon />` - currently displaying CampaignOutlinedIcon +| Tag | Description | +|-----|-------------| +| `<small>` | Renders text in smaller size | +| `<big>` | Renders text in larger size | +| `<terms>` | Formats terms of service links | +| `<privacy>` | Formats privacy policy links | +| `<icon />` | Renders CampaignOutlinedIcon from @mui/icons-material |🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: A comma is probably missing here.
Context: ...vacy (internal links); ##### Regarding formatting you can use the following tags: - `<sm...(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
🪛 Markdownlint
9-9: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (1)
30-34: Consider using theme-based styling.The hardcoded margin and styling values should ideally come from the theme system for better consistency and maintainability.
- icon: <CampaignOutlinedIcon fontSize="small" sx={{ marginRight: '8px', verticalAlign: 'bottom' }} />, + icon: <CampaignOutlinedIcon fontSize="small" sx={(theme) => ({ + marginRight: theme.spacing(1), + verticalAlign: 'bottom' + })} />,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/core/i18n/en/translation.en.json (1 hunks)
- src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.md (1 hunks)
- src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/i18n/en/translation.en.json
🧰 Additional context used
📓 Path-based instructions (2)
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.md (1)
Pattern
**/*.md: Ensure the documentation is clear, concise, and up-to-date, with accurate instructions.src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (1)
Pattern
src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Ensure sufficient error handling and logging is present.
Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
🪛 LanguageTool
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.md
[uncategorized] ~9-~9: A comma is probably missing here.
Context: ...vacy (internal links); ##### Regarding formatting you can use the following tags: - `<sm...(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
🪛 Markdownlint
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.md
3-3: Expected: h4; Actual: h5
Heading levels should only increment by one level at a time(MD001, heading-increment)
3-3: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
9-9: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
🔇 Additional comments (1)
src/main/topLevelPages/myDashboard/releaseNotesBanner/ReleaseNotesBanner.tsx (1)
26-26: LGTM! Clean implementation of the clickable behavior.The implementation now uses a clear and maintainable approach with the isLink prop, as agreed in the previous discussion.
#6988
Based on the requirements the existing Platform Banner was extended to:
releaseNotes.urli18n value;Additional .md was created for ease of understanding the visibility logic and the available configurations behind the feature.
Final design approved by Simone.
Summary by CodeRabbit
New Features
ReleaseNotesBannercomponent to control banner interactivity based on new parameters.Improvements
DashboardBannercomponent for flexible rendering based on a new optional property.ReleaseNotesBannercomponent.Documentation
ReleaseNotesBannercomponent detailing its functionality and usage.