Plugin and advertisement screen revamp #2006
Conversation
WalkthroughThe changes involve a comprehensive redesign of the advertisement and plugin screens in the admin portal. Key updates include renaming labels, enhancing CSS for better styling, improving type safety in TypeScript, and refining component logic. These adjustments aim to align with the new design specifications, ensuring a more consistent and visually appealing user interface. Changes
Assessment against linked issues
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
|
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (12)
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx (3)
Line range hint
77-81: Specify thetypeattribute for the button to ensure correct behavior in forms.+ type="button"
Line range hint
99-99: Add keyboard accessibility to the delete button by handling key events.+ onKeyUp={(event) => { if (event.key === 'Enter') toggleShowDeleteModal(); }}
Line range hint
147-147: Self-close JSX elements that don't have children.- <Card.Img className={styles.admedia} variant="top" src={mediaUrl} data-testid="media"></Card.Img> + <Card.Img className={styles.admedia} variant="top" src={mediaUrl} data-testid="media" />Also applies to: 158-158
src/components/AddOn/core/AddOnStore/AddOnStore.tsx (1)
Line range hint
75-75: Self-close JSX elements that don't have children.- <div className={styles.loader}></div> + <div className={styles.loader} />src/components/Advertisements/Advertisements.tsx (3)
Line range hint
53-53: Use optional chaining to safely access nested properties.- orgAdvertisementListData && orgAdvertisementListData.organizations + orgAdvertisementListData?.organizationsAlso applies to: 67-67
Line range hint
126-128: Self-close JSX elements that don't have children.- <div className={styles.itemCard}></div> + <div className={styles.itemCard} />Also applies to: 130-130, 205-207, 209-209
Line range hint
123-123: Avoid using the index of an array as a key in React lists.- key={index} + key={`ad-${index}`}Also applies to: 176-176, 202-202, 255-255
src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.tsx (5)
Line range hint
217-217: Self-close JSX elements that don't have children.- <video></video> + <video />Also applies to: 312-312
Line range hint
221-221: Add keyboard accessibility to interactive elements by handling key events.+ onKeyUp={(event) => { if (event.key === 'Enter') handleShow(); }}
Line range hint
267-267: Use optional chaining to safely access nested properties.- if (error instanceof Error) { + if (error?.message) {
Line range hint
292-293: Provide alt text for images to improve accessibility.- <img src={formState.advertisementMedia} /> + <img src={formState.advertisementMedia} alt="Advertisement Media" />
Line range hint
295-311: Specify thetypeattribute for buttons to ensure correct behavior in forms.+ type="button"
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (15)
- public/locales/en.json (2 hunks)
- src/components/ActionItems/ActionItemsModal.tsx (2 hunks)
- src/components/AddOn/core/AddOnEntry/AddOnEntry.module.css (1 hunks)
- src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx (5 hunks)
- src/components/AddOn/core/AddOnStore/AddOnStore.module.css (2 hunks)
- src/components/AddOn/core/AddOnStore/AddOnStore.tsx (6 hunks)
- src/components/AddOn/support/services/Plugin.helper.test.ts (1 hunks)
- src/components/AddOn/support/services/Plugin.helper.ts (1 hunks)
- src/components/Advertisements/Advertisements.module.css (2 hunks)
- src/components/Advertisements/Advertisements.tsx (7 hunks)
- src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.module.css (1 hunks)
- src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx (6 hunks)
- src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.module.css (1 hunks)
- src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.tsx (2 hunks)
- src/components/EventCalendar/EventCalendar.module.css (1 hunks)
Files not reviewed due to errors (3)
- src/components/AddOn/core/AddOnStore/AddOnStore.module.css (no review received)
- src/components/AddOn/support/services/Plugin.helper.test.ts (no review received)
- src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx (no review received)
Files skipped from review due to trivial changes (5)
- public/locales/en.json
- src/components/ActionItems/ActionItemsModal.tsx
- src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.module.css
- src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.module.css
- src/components/EventCalendar/EventCalendar.module.css
Additional Context Used
Biome (42)
src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx (1)
102-104: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
src/components/AddOn/core/AddOnStore/AddOnStore.tsx (12)
75-75: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
108-108: Do not use template literals if interpolation and special-character handling are not needed.
144-144: Use === instead of ==.
== is only allowed when comparing againstnull
146-152: This else clause can be omitted because previous branches break early.
159-159: Use === instead of ==.
== is only allowed when comparing againstnull
161-167: This else clause can be omitted because previous branches break early.
202-208: This else clause can be omitted because previous branches break early.
218-218: Use === instead of ==.
== is only allowed when comparing againstnull
220-226: This else clause can be omitted because previous branches break early.
1-1: The default import is only used as a type.
174-174: Avoid using the index of an array as key property in an element.
233-233: Avoid using the index of an array as key property in an element.
src/components/AddOn/support/services/Plugin.helper.ts (1)
22-22: Do not use template literals if interpolation and special-character handling are not needed.
src/components/Advertisements/Advertisements.tsx (16)
38-38: Unexpected any. Specify a different type.
53-53: Change to an optional chain.
67-67: Change to an optional chain.
99-99: Do not use template literals if interpolation and special-character handling are not needed.
126-128: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
130-130: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
130-130: Provide screen reader accessible content when using heading elements.
205-207: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
209-209: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
209-209: Provide screen reader accessible content when using heading elements.
1-1: The default import is only used as a type.
52-52: This hook does not specify all of its dependencies: advertisements
123-123: Avoid using the index of an array as key property in an element.
176-176: Avoid using the index of an array as key property in an element.
202-202: Avoid using the index of an array as key property in an element.
255-255: Avoid using the index of an array as key property in an element.
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx (5)
99-99: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
147-147: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
158-158: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
74-74: Avoid using the index of an array as key property in an element.
77-81: Provide an explicit type prop for the button element.
src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.tsx (7)
217-217: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
221-221: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
267-267: Change to an optional chain.
292-293: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
312-312: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
1-1: The default import is only used as a type.
295-311: Provide an explicit type prop for the button element.
Additional comments not posted (9)
src/components/AddOn/core/AddOnEntry/AddOnEntry.module.css (2)
10-11: Updated background and text color for.entryactionclass enhances visual consistency and appeal.
14-14: Green border addition to.cardclass aligns with the redesign objectives to make UI elements more visually appealing.src/components/Advertisements/Advertisements.module.css (3)
4-10: Updated.listBoxto use grid layout enhances alignment and spacing, improving the visual structure of the component.
21-25: Updated.inputclass with specific width and position settings enhances control over the layout and appearance.
26-30: Updated.justifyspto use grid layout enhances alignment and spacing, improving the visual structure of the component.src/components/AddOn/support/services/Plugin.helper.ts (3)
1-12: Introduction ofInterfacePluginHelperenhances type safety and clarity in handling plugin data.
26-31: Refinement ofgenerateLinksto useInterfacePluginHelperenhances data handling and consistency.
15-18: Ensure type consistency by updating the return type offetchStoretoPromise<InterfacePluginHelper[]>.- const result = await fetch(`http://localhost:${port}/store`); + const result = await fetch('http://localhost:4321/store');Avoid using template literals when not necessary for interpolation.
Likely invalid or redundant comment.
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx (1)
21-21: Ensure thesetAfterfunction is properly documented and tested, especially since it's part of the component's interface.
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx
Outdated
Show resolved
Hide resolved
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx
Show resolved
Hide resolved
|
Please make the suggested code rabbit updates to the code |
|
Please fix the conflicting files and address the coderabbit suggestions |
|
sorry for late response i'll make the suggested changes |
|
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
|
Please fix the conflicting files. |
|
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
|
Closing. Inactivity and many conflicting files |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx (3)
62-62: Remove commented console.log statementDebug statements should be removed before committing to production code.
- // console.log(currentOrg);
105-108: Move inline styles to CSS moduleConsider moving the Card's inline styles to the existing
AddOnEntry.module.cssfile for better maintainability and consistency.- <Card - data-testid="AddOnEntry" - style={{ border: '1px solid #31BB6B', borderRadius: '10px' }} - > + <Card + data-testid="AddOnEntry" + className={styles.addOnCard} + >Add to AddOnEntry.module.css:
.addOnCard { border: 1px solid #31BB6B; border-radius: 10px; }
121-121: Move title font weight to CSS moduleConsider moving the Card.Title's inline style to the CSS module for consistency.
- <Card.Title style={{ fontWeight: '800' }}>{title}</Card.Title> + <Card.Title className={styles.cardTitle}>{title}</Card.Title>Add to AddOnEntry.module.css:
.cardTitle { font-weight: 800; }src/components/Advertisements/Advertisements.tsx (2)
103-103: Use translation for placeholder textThe placeholder text should use the translation system for consistency with the rest of the application.
-placeholder={'Search..'} +placeholder={t('searchPlaceholder')}
Line range hint
126-275: Reduce code duplicationThe InfiniteScroll implementation and date filtering logic is duplicated between the active and archived tabs. Consider:
- Creating a reusable component for the InfiniteScroll implementation
- Extracting the date comparison logic into a utility function
Example refactor for the date logic:
const isAdvertisementActive = (ad: Ad): boolean => new Date(ad.endDate) > new Date(); const isAdvertisementArchived = (ad: Ad): boolean => !isAdvertisementActive(ad); // Usage in JSX: advertisements.filter(isAdvertisementActive).map(...) advertisements.filter(isAdvertisementArchived).map(...)src/components/AddOn/core/AddOnStore/AddOnStore.tsx (2)
45-45: Remove unused state variable for cleaner code.The state variable
dataListis not used anywhere in the code. Removing it can simplify the component and reduce unnecessary state management.Apply this diff to remove the unused state variable:
- const [, setDataList] = useState<InterfacePluginHelper[]>([]); + // Removed unused state variable
232-232: Use strict equality checks (===) for accurate comparisons.Replace
==with===when comparingsearchTextto ensure strict type comparison and avoid unexpected behavior.Apply this diff:
- if (searchText == '') { + if (searchText === '') {Also applies to lines: 176, 233
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
public/locales/en/translation.json(2 hunks)src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx(5 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.tsx(9 hunks)src/components/Advertisements/Advertisements.tsx(5 hunks)src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx(3 hunks)src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.module.css(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/locales/en/translation.json
🔇 Additional comments (4)
src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx (2)
22-22: LGTM: Improved type safety
The change from any to void return type for getInstalledPlugins is a good improvement in type safety.
Line range hint 1-153: Add test coverage for the UI changes
The PR objectives mention that no tests were added for these changes. Consider adding tests for:
- Visual regression tests for the new styling
- Unit tests for the installation/uninstallation logic
- Integration tests for the plugin management workflow
Would you like help creating the test cases?
src/components/Advertisements/Advertisements.tsx (2)
237-237: Verify translation key exists
The 'endOfResults' translation key has been moved from the common namespace to the advertisement namespace. Please ensure this key exists in the advertisement translations.
✅ Verification successful
Translation key exists in both namespaces
The 'endOfResults' translation key exists in both the common namespace (common.json) and the default namespace (translation.json) for all supported languages (en, fr, hi, sp, zh). The change from tCommon('endOfResults') to t('endOfResults') is safe as the key is present in both locations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the translation key exists in the advertisement namespace
fd -e json . public/locales | xargs rg '"endOfResults"'
Length of output: 1823
122-126:
Fix spelling in tab key
The tab key 'archievedAds' contains a spelling error.
-eventKey="archievedAds"
+eventKey="archivedAds"Likely invalid or redundant comment.
src/components/Advertisements/core/AdvertisementRegister/AdvertisementRegister.module.css
Show resolved
Hide resolved
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx
Show resolved
Hide resolved
src/components/Advertisements/core/AdvertisementEntry/AdvertisementEntry.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx (2)
323-334: LGTM! Consider enhancing test coverage.The updated test assertions correctly reflect the new dropdown-based filter UI. However, consider adding these test scenarios for more comprehensive coverage:
- Verify that selecting different filter options updates the displayed plugins
- Test edge cases like rapid toggling of the dropdown
- Ensure proper keyboard navigation support
Add these test cases:
// Test filter functionality test('filters plugins correctly when dropdown option is selected', async () => { // ... setup code ... const dropdownButton = screen.getByRole('button', { name: /enabled/i }); userEvent.click(dropdownButton); // Select "Disabled" option userEvent.click(screen.getByText(/disabled/i)); // Verify only disabled plugins are shown expect(screen.queryByText('EnabledPlugin')).not.toBeInTheDocument(); expect(screen.getByText('DisabledPlugin')).toBeInTheDocument(); }); // Test keyboard navigation test('supports keyboard navigation in dropdown', async () => { // ... setup code ... const dropdownButton = screen.getByRole('button', { name: /enabled/i }); // Open dropdown with Enter key dropdownButton.focus(); fireEvent.keyDown(dropdownButton, { key: 'Enter' }); // Verify dropdown options are focusable expect(screen.getByText(/disabled/i)).toHaveFocus(); });
Line range hint
1-424: Consider enhancing overall test structure and coverage.While the test file is well-organized, consider these improvements for better maintainability and coverage:
- Extract mock data to a separate file to improve readability
- Add more varied test cases for error scenarios
- Enhance the loading test to verify loading states for both tabs
Here's a suggested approach for the loading test:
test('displays loading states correctly', async () => { const mocks = [ORGANIZATIONS_LIST_MOCK, PLUGIN_LOADING_MOCK]; render( // ... setup code ... ); // Verify initial loading state expect(screen.getByTestId('AddOnEntryStore')).toBeInTheDocument(); expect(screen.getByRole('progressbar')).toBeInTheDocument(); // Switch to Installed tab userEvent.click(screen.getByText('Installed')); expect(screen.getByRole('progressbar')).toBeInTheDocument(); // Wait for loading to complete await wait(); expect(screen.queryByRole('progressbar')).not.toBeInTheDocument(); });Consider moving mock data to a separate file:
// __mocks__/addOnStore.mocks.ts export const PLUGIN_GET_MOCK = { // ... existing mock data ... }; export const ORGANIZATIONS_LIST_MOCK = { // ... existing mock data ... };src/components/Advertisements/Advertisements.test.tsx (4)
464-464: Consider making date validation tests more robust.The change from
date[1]todate[0]suggests a dependency on array order. Consider making the test more resilient by:
- Adding explicit assertions about the expected number of date elements
- Using a more specific test ID or data attribute to target the exact date element
- const dateString = date[0].innerHTML; + expect(date).toHaveLength(2); // Verify expected number of dates + const dateString = date.find(el => el.getAttribute('data-type') === 'end-date')?.innerHTML; + expect(dateString).toBeDefined(); // Ensure we found the correct date element
Line range hint
676-677: Remove console.log statements from test code.Debug logging statements should not be committed to test files as they add noise to test output.
- console.log('before scroll', moreiconbtn); - fireEvent.scroll(window, { target: { scrollY: 500 } }); + fireEvent.scroll(window, { target: { scrollY: 500 } });
Line range hint
679-679: Remove console.log statement.Debug logging statement should be removed.
- console.log('after scroll', moreiconbtn);
Line range hint
462-482: Consider using a mock date to make tests deterministic.The test relies on comparing dates with the current time, which can make tests non-deterministic. Consider using a mocked date for consistent test results.
+ // Mock current date to ensure consistent test results + const mockCurrentDate = new Date('2024-01-01'); + jest.spyOn(global, 'Date').mockImplementation(() => mockCurrentDate); const date = await screen.findAllByTestId('Ad_end_date'); const dateString = date[0].innerHTML; const dateMatch = dateString.match( /\b(?:Sun|Mon|Tue|Wed|Thu|Fri|Sat)\s+(Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)\s+(\d{1,2})\s+(\d{4})\b/, ); let dateObject = new Date(); if (dateMatch) { const monthName = dateMatch[1]; const day = parseInt(dateMatch[2], 10); const year = parseInt(dateMatch[3], 10); const monthIndex = 'JanFebMarAprMayJunJulAugSepOctNovDec'.indexOf(monthName) / 3; dateObject = new Date(year, monthIndex, day); } - expect(dateObject.getTime()).toBeLessThan(new Date().getTime()); + expect(dateObject.getTime()).toBeLessThan(mockCurrentDate.getTime()); + + // Restore Date after test + jest.restoreAllMocks();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx(1 hunks)src/components/Advertisements/Advertisements.test.tsx(1 hunks)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2006 +/- ##
===========================================
- Coverage 98.26% 98.26% -0.01%
===========================================
Files 297 297
Lines 8639 8628 -11
Branches 2518 2475 -43
===========================================
- Hits 8489 8478 -11
Misses 139 139
Partials 11 11 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/components/AddOn/core/AddOnStore/AddOnStore.tsx (2)
102-113: Enhance robustness of plugin filtering.The current implementation has potential issues with null safety and case handling.
Consider this improved implementation:
const filterPlugins = ( plugins: InterfacePluginHelper[], searchTerm: string, ): InterfacePluginHelper[] => { - if (!searchTerm) { + if (!searchTerm.trim()) { return plugins; } + const normalizedSearch = searchTerm.toLowerCase().trim(); return plugins.filter((plugin) => - plugin.pluginName?.toLowerCase().includes(searchTerm.toLowerCase()), + plugin.pluginName.toLowerCase().includes(normalizedSearch), ); };
126-141: Move inline styles to CSS modules.Inline styles make the code harder to maintain and don't support media queries or pseudo-classes. Consider moving these styles to your CSS module.
Move the styles to
AddOnStore.module.css:.container { background-color: white; margin: 2px; padding: 10px; border-radius: 20px; } .headerCol { display: flex; align-items: center; justify-content: space-between; }Then update the JSX:
- <Row - className="" - style={{ - backgroundColor: 'white', - margin: '2px', - padding: '10px', - borderRadius: '20px', - }} - > + <Row className={styles.container}> - <Col - style={{ - display: 'flex', - alignItems: 'center', - justifyContent: 'space-between', - }} - > + <Col className={styles.headerCol}>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx(2 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.tsx(7 hunks)src/components/Advertisements/Advertisements.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/AddOn/core/AddOnStore/AddOnStore.test.tsx
- src/components/Advertisements/Advertisements.tsx
What kind of change does this PR introduce?
Redesign
Issue Number:
Fixes #1918
Did you add tests for your changes?
No
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
Summary by CodeRabbit
New Features
Bug Fixes
Style
Refactor