-
-
Notifications
You must be signed in to change notification settings - Fork 875
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Figma: Refactoring Events Dashboard / Detail Screens #3346
Figma: Refactoring Events Dashboard / Detail Screens #3346
Conversation
Warning Rate limit exceeded@aadhil2k4 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request focuses on enhancing the CSS styling and test coverage across multiple components in the Talawa Admin application. The changes primarily involve adding styling tests, updating class names, and consolidating CSS into the global Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/screens/EventManagement/EventManagement.tsx (1)
Line range hint
224-281
: Extract repeated container styling.The same className is repeated across all tab containers. Consider extracting it to a constant or CSS module class.
+ const tabContainerClass = "mx-4 bg-white p-4 pt-2 rounded-4 shadow"; + {(() => { switch (tab) { case 'dashboard': return ( <div data-testid="eventDashboardTab" - className="mx-4 bg-white p-4 pt-2 rounded-4 shadow" + className={tabContainerClass} > <EventDashboard eventId={eventId} /> </div> ); // Apply similar changes to other cases
🧹 Nitpick comments (9)
src/components/CheckIn/CheckInWrapper.spec.tsx (2)
98-117
: Consider using more reliable style testing approaches.The computed style test may be flaky as it depends on browser rendering. Instead of checking the computed
marginTop
value, consider:
- Verifying the presence of utility classes that define the margin
- Using snapshot testing for style verification
- const styles = window.getComputedStyle(button); - expect(button).toHaveClass('mt-4'); - expect(styles.marginTop).not.toBe('0px'); + expect(button).toHaveClass('mt-4');
119-142
: Improve layout testing reliability.The current approach of using string index comparison for layout verification is fragile. Consider using DOM traversal methods or test-ids for more reliable layout testing.
- expect( - button.innerHTML.indexOf('Sort') < - button.innerHTML.indexOf('Check In Registrants'), - ).toBe(true); + const buttonContent = Array.from(button.childNodes); + const imageIndex = buttonContent.findIndex(node => + node.nodeName === 'IMG' && node.getAttribute('alt') === 'Sort' + ); + const textIndex = buttonContent.findIndex(node => + node.textContent?.includes('Check In Registrants') + ); + expect(imageIndex).toBeLessThan(textIndex);src/components/EventManagement/Dashboard/EventDashboard.spec.tsx (2)
116-117
: Improve type safety in cards query.The
querySelectorAll
result needs type assertion for better type safety.- const cards = statsSection.querySelectorAll(`.${styles.ctacards}`); + const cards = Array.from(statsSection.querySelectorAll<HTMLElement>(`.${styles.ctacards}`)); expect(cards).toHaveLength(3);
189-191
: Enhance color verification.Instead of just checking for the presence of
text-success
class, consider verifying the computed color value for more robust testing.const statusText = recurringStatus.querySelector('.text-success'); expect(statusText).toBeInTheDocument(); - expect(statusText).toHaveClass('ml-2'); + expect(statusText).toHaveClass('text-success', 'ml-2'); + // Optional: Verify the actual color if critical + // const computedStyle = window.getComputedStyle(statusText as HTMLElement); + // expect(computedStyle.color).toBe('rgb(25, 135, 84)'); // Bootstrap success colorsrc/screens/EventManagement/EventManagement.tsx (1)
138-144
: Simplify button styling logic.The conditional class name assignment could be simplified using template literals and optional classes.
- const variant = selected ? 'secondary' : 'light'; - const className = selected - ? `px-4 d-flex align-items-center rounded-3 shadow-sm ${styles.createButton}` - : 'text-secondary bg-white px-4 d-flex align-items-center rounded-3 shadow-sm'; + const baseClasses = 'px-4 d-flex align-items-center rounded-3 shadow-sm'; + const variant = selected ? 'secondary' : 'light'; + const className = `${baseClasses} ${selected ? styles.createButton : 'text-secondary bg-white'}`;src/style/app.module.css (1)
1881-1883
: Consider removing commented code.The commented-out hover state for radio buttons should either be:
- Removed if no longer needed
- Uncommented if still required
- Documented if kept for future reference
src/components/EventManagement/EventRegistrant/EventRegistrants.spec.tsx (3)
266-283
: Consider enhancing the filter button styling test.While the test covers class name verification, consider these improvements:
- Add async/await pattern as the component performs async operations
- Add actual CSS property verification using
getComputedStyle
- Consider adding a snapshot test for style changes
- test('Filter button has correct styling classes', () => { + test('Filter button has correct styling classes', async () => { render( <MockedProvider addTypename={false} link={link}> <BrowserRouter> <Provider store={store}> <I18nextProvider i18n={i18n}> <EventRegistrants /> </I18nextProvider> </Provider> </BrowserRouter> </MockedProvider>, ); + await wait(); const filterButton = screen.getByTestId('filter-button'); expect(filterButton).toHaveClass('border-1'); expect(filterButton).toHaveClass('mx-4'); expect(filterButton).toHaveClass('mt-4'); expect(filterButton).toHaveClass(styles.createButton); + + // Verify actual styling properties + const computedStyle = window.getComputedStyle(filterButton); + expect(computedStyle.marginTop).toBe('1rem'); + expect(computedStyle.borderWidth).toBe('1px'); });
302-318
: Enhance custom cell styling test coverage.Consider improving the test by:
- Adding async/await pattern
- Verifying specific header cells
- Checking actual styling properties
- test('Table cells have custom cell styling', () => { + test('Table cells have custom cell styling', async () => { render( <MockedProvider addTypename={false} link={link}> <BrowserRouter> <Provider store={store}> <I18nextProvider i18n={i18n}> <EventRegistrants /> </I18nextProvider> </Provider> </BrowserRouter> </MockedProvider>, ); + await wait(); const headerCells = screen.getAllByRole('columnheader'); + const expectedHeaders = ['Serial', 'Registrant', 'Created At', 'Add Registrant']; + + // Verify specific headers + headerCells.forEach((cell, index) => { + expect(cell).toHaveTextContent(expectedHeaders[index]); + expect(cell).toHaveClass(styles.customcell); + + // Verify actual styling + const computedStyle = window.getComputedStyle(cell); + expect(computedStyle.padding).toBeDefined(); + }); - headerCells.forEach((cell) => { - expect(cell).toHaveClass(styles.customcell); - }); });
265-336
: Overall feedback on new test additions.The new tests are valuable additions that verify styling and accessibility requirements. However, there are consistent patterns that could be improved across all tests:
- Add async/await patterns for component rendering
- Verify actual CSS properties using
getComputedStyle
- Enhance accessibility testing coverage
- Use more robust selectors
These improvements will make the tests more reliable and comprehensive.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
public/images/svg/attendees.svg
is excluded by!**/*.svg
public/images/svg/feedback.svg
is excluded by!**/*.svg
public/images/svg/options-outline.svg
is excluded by!**/*.svg
public/images/svg/organization.svg
is excluded by!**/*.svg
📒 Files selected for processing (14)
src/components/CheckIn/CheckInWrapper.spec.tsx
(1 hunks)src/components/CheckIn/CheckInWrapper.tsx
(1 hunks)src/components/EventManagement/Dashboard/EventDashboard.spec.tsx
(2 hunks)src/components/EventManagement/Dashboard/EventDashboard.tsx
(1 hunks)src/components/EventManagement/EventAttendance/EventAttendance.spec.tsx
(2 hunks)src/components/EventManagement/EventAttendance/EventAttendance.tsx
(2 hunks)src/components/EventManagement/EventRegistrant/EventRegistrants.spec.tsx
(2 hunks)src/components/EventManagement/EventRegistrant/EventRegistrants.tsx
(2 hunks)src/screens/EventManagement/EventManagement.tsx
(4 hunks)src/screens/EventVolunteers/Requests/Requests.spec.tsx
(2 hunks)src/screens/EventVolunteers/Requests/Requests.tsx
(1 hunks)src/screens/EventVolunteers/VolunteerGroups/VolunteerGroups.tsx
(2 hunks)src/screens/EventVolunteers/Volunteers/Volunteers.tsx
(2 hunks)src/style/app.module.css
(3 hunks)
✅ Files skipped from review due to trivial changes (7)
- src/components/CheckIn/CheckInWrapper.tsx
- src/components/EventManagement/Dashboard/EventDashboard.tsx
- src/screens/EventVolunteers/Requests/Requests.tsx
- src/components/EventManagement/EventRegistrant/EventRegistrants.tsx
- src/screens/EventVolunteers/Volunteers/Volunteers.tsx
- src/components/EventManagement/EventAttendance/EventAttendance.tsx
- src/screens/EventVolunteers/VolunteerGroups/VolunteerGroups.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (4)
src/screens/EventVolunteers/Requests/Requests.spec.tsx (1)
240-304
: Well-structured test suite for styling!The new test suite for styling verification is well-organized with:
- Reusable component rendering logic
- Clear test cases for each styled element
- Specific assertions for CSS classes and styles
src/style/app.module.css (2)
823-824
: Consistent color scheme updates for interactive elements.The color scheme changes for toggle buttons and radio buttons maintain consistency by:
- Using shared color variables (
--search-button-bg
,--blue-primary
)- Applying consistent border styles
- Maintaining visual hierarchy through color contrast
Also applies to: 871-872, 1876-1878
1809-1810
: Consistent table header styling.The table header styling aligns with the application's color scheme by using the search button background color and maintaining readability with black text.
src/components/EventManagement/EventRegistrant/EventRegistrants.spec.tsx (1)
16-16
: LGTM! Import statement aligns with CSS consolidation strategy.The import of styles from the global CSS module aligns with the project's strategy of consolidating styles into
app.module.css
.
src/components/EventManagement/EventAttendance/EventAttendance.spec.tsx
Outdated
Show resolved
Hide resolved
const searchContainer = screen.getByTestId('searchByName').parentElement; | ||
expect(searchContainer).toHaveClass(styles.input, 'me-3'); | ||
|
||
const searchInput = screen.getByTestId('searchByName'); | ||
expect(searchInput).toHaveClass('bg-white', 'border'); | ||
|
||
const searchButton = searchContainer?.querySelector('button'); | ||
expect(searchButton).toHaveClass( | ||
'position-absolute', | ||
'z-10', | ||
'bottom-0', | ||
'end-0', | ||
'h-100', | ||
styles.regularBtn, | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add null checks for element queries.
The parent element and button queries could return null. Add appropriate null checks to prevent potential runtime errors.
const searchContainer = screen.getByTestId('searchByName').parentElement;
+ expect(searchContainer).not.toBeNull();
expect(searchContainer).toHaveClass(styles.input, 'me-3');
const searchInput = screen.getByTestId('searchByName');
expect(searchInput).toHaveClass('bg-white', 'border');
const searchButton = searchContainer?.querySelector('button');
+ expect(searchButton).not.toBeNull();
expect(searchButton).toHaveClass(
'position-absolute',
'z-10',
'bottom-0',
'end-0',
'h-100',
styles.regularBtn,
);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const searchContainer = screen.getByTestId('searchByName').parentElement; | |
expect(searchContainer).toHaveClass(styles.input, 'me-3'); | |
const searchInput = screen.getByTestId('searchByName'); | |
expect(searchInput).toHaveClass('bg-white', 'border'); | |
const searchButton = searchContainer?.querySelector('button'); | |
expect(searchButton).toHaveClass( | |
'position-absolute', | |
'z-10', | |
'bottom-0', | |
'end-0', | |
'h-100', | |
styles.regularBtn, | |
); | |
}); | |
const searchContainer = screen.getByTestId('searchByName').parentElement; | |
expect(searchContainer).not.toBeNull(); | |
expect(searchContainer).toHaveClass(styles.input, 'me-3'); | |
const searchInput = screen.getByTestId('searchByName'); | |
expect(searchInput).toHaveClass('bg-white', 'border'); | |
const searchButton = searchContainer?.querySelector('button'); | |
expect(searchButton).not.toBeNull(); | |
expect(searchButton).toHaveClass( | |
'position-absolute', | |
'z-10', | |
'bottom-0', | |
'end-0', | |
'h-100', | |
styles.regularBtn, | |
); | |
}); |
test('Table container has correct styling', () => { | ||
render( | ||
<MockedProvider addTypename={false} link={link}> | ||
<BrowserRouter> | ||
<Provider store={store}> | ||
<I18nextProvider i18n={i18n}> | ||
<EventRegistrants /> | ||
</I18nextProvider> | ||
</Provider> | ||
</BrowserRouter> | ||
</MockedProvider>, | ||
); | ||
const tableContainer = screen.getByRole('grid').closest('.mt-4'); | ||
expect(tableContainer).toHaveClass('mt-4'); | ||
expect(tableContainer).toHaveClass(styles.tableHead); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve robustness of table container styling test.
The current implementation has some potential issues:
- Using
closest('.mt-4')
is fragile and could break if class names change - Missing async/await pattern for component rendering
- No verification of actual margin-top styling
- test('Table container has correct styling', () => {
+ test('Table container has correct styling', async () => {
render(
<MockedProvider addTypename={false} link={link}>
<BrowserRouter>
<Provider store={store}>
<I18nextProvider i18n={i18n}>
<EventRegistrants />
</I18nextProvider>
</Provider>
</BrowserRouter>
</MockedProvider>,
);
+ await wait();
- const tableContainer = screen.getByRole('grid').closest('.mt-4');
+ const tableContainer = screen.getByRole('grid').parentElement;
expect(tableContainer).toHaveClass('mt-4');
expect(tableContainer).toHaveClass(styles.tableHead);
+
+ // Verify actual styling
+ const computedStyle = window.getComputedStyle(tableContainer);
+ expect(computedStyle.marginTop).toBe('1rem');
});
Committable suggestion skipped: line range outside the PR's diff.
test('Table has correct ARIA attributes', () => { | ||
render( | ||
<MockedProvider addTypename={false} link={link}> | ||
<BrowserRouter> | ||
<Provider store={store}> | ||
<I18nextProvider i18n={i18n}> | ||
<EventRegistrants /> | ||
</I18nextProvider> | ||
</Provider> | ||
</BrowserRouter> | ||
</MockedProvider>, | ||
); | ||
const table = screen.getByRole('grid'); | ||
const firstHeaderCell = screen.getByTestId('table-header-serial'); | ||
expect(table).toHaveAttribute('aria-label'); | ||
expect(firstHeaderCell).toHaveAttribute('aria-sort', 'none'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance accessibility testing coverage.
While the test checks for ARIA attributes, consider these improvements:
- Add async/await pattern
- Verify the actual aria-label value
- Check aria-sort for all sortable columns
- test('Table has correct ARIA attributes', () => {
+ test('Table has correct ARIA attributes', async () => {
render(
<MockedProvider addTypename={false} link={link}>
<BrowserRouter>
<Provider store={store}>
<I18nextProvider i18n={i18n}>
<EventRegistrants />
</I18nextProvider>
</Provider>
</BrowserRouter>
</MockedProvider>,
);
+ await wait();
const table = screen.getByRole('grid');
- const firstHeaderCell = screen.getByTestId('table-header-serial');
- expect(table).toHaveAttribute('aria-label');
- expect(firstHeaderCell).toHaveAttribute('aria-sort', 'none');
+ expect(table).toHaveAttribute('aria-label', 'Event Registrants Table');
+
+ // Check all sortable columns
+ const sortableHeaders = [
+ 'table-header-serial',
+ 'table-header-registrant',
+ 'table-header-created-at'
+ ];
+
+ sortableHeaders.forEach(headerId => {
+ const header = screen.getByTestId(headerId);
+ expect(header).toHaveAttribute('aria-sort', 'none');
+ });
});
Committable suggestion skipped: line range outside the PR's diff.
….spec.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3346 +/- ##
=====================================================
+ Coverage 8.43% 89.87% +81.43%
=====================================================
Files 310 331 +21
Lines 8069 8583 +514
Branches 1792 1901 +109
=====================================================
+ Hits 681 7714 +7033
+ Misses 7311 608 -6703
- Partials 77 261 +184
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
All you have done is changed the colors without making your changes look exactly like Figma: You have not read the issue requirements closely. Examples for every screen are provided.
I have to close this. Please try again. |
What kind of change does this PR introduce?
Refactoring, testing
Issue Number:
Fixes #3182
Snapshots/Videos:
EventManagementDashboard.mp4
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Agendas screen was not loading. So didnt make any changes to it.
Have you read the contributing guide?
Yes
Summary by CodeRabbit
Release Notes
Style Improvements
Test Coverage
User Interface