Filter and Dropdown Buttons replaced with Customized Sorting Button #3225
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of dropdown and filter components across multiple files in the Talawa Admin project. The primary change is the introduction of a new Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Finishing Touches
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
src/screens/BlockUser/BlockUser.spec.tsx (1)
Line range hint
1-524: Add test coverage for new sorting functionality.While the test file maintains good coverage for existing features, it should be extended to include test cases for the new sorting functionality that replaced the dropdown buttons. Consider adding tests for:
- Sorting by different criteria
- Toggling sort order
- Edge cases in sorting behavior
This will ensure the new UI changes are thoroughly tested.
Would you like me to help generate the additional test cases for the sorting functionality?
src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.spec.tsx (1)
Test IDs 'all' and 'disabled' are missing in component implementation
The test is using
data-testid="all"anddata-testid="disabled"to find filter elements, but these test IDs are not present in the component implementation. This will cause the test to fail.
src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.spec.tsx: Update test IDs to match the actual implementation or add the missing test IDs to the component.🔗 Analysis chain
Line range hint
128-146: LGTM! Verify corresponding component changes.The test ID changes from 'statusAll'/'statusDisabled' to 'all'/'disabled' align with the new sorting button implementation.
Run this script to verify the component implementation matches these test IDs:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the component implementation uses the updated test IDs ast-grep --pattern 'data-testid={$_("all" | "disabled")}'Length of output: 59
Script:
#!/bin/bash # Search for data-testid usage with context rg -U "data-testid.*['\"](?:all|disabled)['\"]" --type ts --type tsx -A 2 -B 2 # Broader search for these specific test IDs rg -U "(?:all|disabled).+data-testid|data-testid.+(?:all|disabled)" --type ts --type tsx -A 2 -B 2Length of output: 241
Script:
#!/bin/bash # Find the component file fd "OrgActionItemCategories" --type f # Search for data-testid in found files rg "data-testid" -B 2 -A 2 src/components/OrgSettings/ActionItemCategories/Length of output: 5796
🧹 Nitpick comments (21)
src/screens/OrganizationActionItems/OrganizationActionItems.spec.tsx (1)
255-259: Document the test ID changes for future maintenance.Consider adding a comment block at the beginning of the test file to document the mapping between old and new test IDs. This will help future maintainers understand the changes and maintain consistency.
Add this documentation at the start of the test file:
+ /** + * Test ID Mappings after SortingButton Implementation: + * - statusAll → all + * - statusPending → pending + * - statusCompleted → completed + */Also applies to: 272-276, 317-321
src/subComponents/SortingButton.tsx (2)
8-23: Add JSDoc documentation for interfacesConsider adding JSDoc documentation to describe the purpose and usage of each interface and its properties. This will improve code maintainability and help other developers understand how to use these interfaces correctly.
+/** + * Represents a single option in the sorting dropdown menu. + */ interface InterfaceSortingOption { label: string; value: string; } +/** + * Props for the SortingButton component. + * @property {string} [title] - Optional title for the dropdown button + * @property {InterfaceSortingOption[]} sortingOptions - Array of available sorting options + * @property {string} [selectedOption] - Currently selected option's value + * @property {(value: string) => void} onSortChange - Callback when a sorting option is selected + * @property {string} dataTestIdPrefix - Prefix for data-testid attributes + * @property {string} [dropdownTestId] - Optional data-testid for the dropdown container + * @property {string} [className] - Optional CSS class name + * @property {string} [buttonLabel] - Optional custom label for the button + * @property {'sort' | 'filter'} [type] - Type of button to determine the icon + */ interface InterfaceSortingButtonProps {
66-80: Add missing PropType for classNameThe
classNameprop is defined in the TypeScript interface but missing in PropTypes validation.SortingButton.propTypes = { title: PropTypes.string, sortingOptions: PropTypes.arrayOf( PropTypes.exact({ label: PropTypes.string.isRequired, value: PropTypes.string.isRequired, }).isRequired, ).isRequired, selectedOption: PropTypes.string, onSortChange: PropTypes.func.isRequired, dataTestIdPrefix: PropTypes.string.isRequired, dropdownTestId: PropTypes.string, buttonLabel: PropTypes.string, type: PropTypes.oneOf(['sort', 'filter']), + className: PropTypes.string, };src/screens/OrgSettings/OrgSettings.tsx (1)
Line range hint
59-71: Consider mobile-friendly navigation alternativesWith the removal of the dropdown menu, the button-only navigation might not scale well on mobile devices with many settings categories. Consider implementing a more mobile-friendly navigation pattern, such as a collapsible menu for smaller screens.
src/components/EventManagement/EventAttendance/EventAttendance.spec.tsx (2)
106-109: Add data-testid attributes for more reliable test selectors.The test relies on text content ("Ascending") to find elements, which could break if the text changes. Consider using data-testid attributes for more reliable test selectors.
- const sortOption = screen.getByText('Ascending'); + const sortOption = screen.getByTestId('sort-ascending-option');
123-130: Enhance test coverage for filter functionality.The test only verifies the "no attendees" case. Consider adding test cases for:
- When attendees are found
- Different filter options
- Edge cases like empty states
src/screens/OrganizationVenues/OrganizationVenues.tsx (1)
167-191: Consider extracting sorting options to constants.The sorting options arrays could be moved to constants to improve maintainability and reusability.
+const SEARCH_OPTIONS = [ + { label: tCommon('name'), value: 'name' }, + { label: tCommon('description'), value: 'desc' }, +]; + +const SORT_OPTIONS = [ + { label: t('highestCapacity'), value: 'highest' }, + { label: t('lowestCapacity'), value: 'lowest' }, +]; - <SortingButton - sortingOptions={[ - { label: tCommon('name'), value: 'name' }, - { label: tCommon('description'), value: 'desc' }, - ]} + <SortingButton + sortingOptions={SEARCH_OPTIONS} ... />src/screens/UserPortal/Volunteer/Invitations/Invitations.spec.tsx (1)
Line range hint
174-213: Consider using test data constants and improving test organization.
- Extract test IDs to constants for better maintainability
- Consider grouping related tests using
describeblocks- Add test cases for edge cases and error scenarios
+const TEST_IDS = { + FILTER_ALL: 'all', + FILTER_GROUP: 'group', + FILTER_INDIVIDUAL: 'individual', +}; +describe('Filter functionality', () => { it('Filter Invitations (all)', async () => { - const filterAll = await screen.findByTestId('all'); + const filterAll = await screen.findByTestId(TEST_IDS.FILTER_ALL); ... }); ... });src/screens/UserPortal/Campaigns/Campaigns.tsx (1)
154-173: Simplify type handling for better maintainability.While the implementation is functionally correct, the type handling can be improved:
Consider defining a union type for sort options:
type CampaignSortBy = 'fundingGoal_ASC' | 'fundingGoal_DESC' | 'endDate_ASC' | 'endDate_DESC';Then simplify the implementation:
- onSortChange={(value) => - setSortBy( - value as - | 'fundingGoal_ASC' - | 'fundingGoal_DESC' - | 'endDate_ASC' - | 'endDate_DESC', - ) - } + onSortChange={(value) => setSortBy(value as CampaignSortBy)}src/screens/BlockUser/BlockUser.tsx (1)
219-251: LGTM! Consider extracting sorting options for better maintainability.The implementation of
SortingButtoncomponents effectively replaces the previous dropdown functionality while maintaining all features. The components are well-integrated with translations and include appropriate test IDs.Consider extracting the sorting options into constants at the top of the file for better maintainability:
const USER_FILTER_OPTIONS = [ { label: t('allMembers'), value: 'allMembers' }, { label: t('blockedUsers'), value: 'blockedUsers' }, ]; const NAME_FILTER_OPTIONS = [ { label: t('searchByFirstName'), value: 'searchByFirstName' }, { label: t('searchByLastName'), value: 'searchByLastName' }, ];src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.tsx (1)
323-343: Consider simplifying the selectedOption logic.The implementation is correct, but the selectedOption logic could be simplified for better readability.
Consider extracting the status mapping to a constant:
const STATUS_LABEL_MAP = { [null]: tCommon('all'), [CategoryStatus.Active]: tCommon('active'), [CategoryStatus.Disabled]: tCommon('disabled'), }; // Then simplify the selectedOption prop: selectedOption={STATUS_LABEL_MAP[status]}src/screens/EventVolunteers/VolunteerGroups/VolunteerGroups.tsx (1)
325-334: Consider improving the search criteria button label.The button label is currently using
tCommon('searchBy', { item: '' })which might result in an incomplete or unclear label.Consider either:
- Removing the empty
itemparameter:-buttonLabel={tCommon('searchBy', { item: '' })} +buttonLabel={tCommon('searchBy')}
- Or using a more descriptive label:
-buttonLabel={tCommon('searchBy', { item: '' })} +buttonLabel={t('searchCriteria')}src/screens/OrganizationPeople/OrganizationPeople.tsx (2)
279-281: Consider using an enum for role state management.The current implementation uses magic numbers (0, 1, 2) for role states, which could lead to maintenance issues.
Consider introducing an enum:
enum RoleState { Members = 0, Admins = 1, Users = 2 }Then update the handler:
-const handleSortChange = (value: string): void => { - setState(value === 'users' ? 2 : value === 'members' ? 0 : 1); +const handleSortChange = (value: string): void => { + setState(value === 'users' ? RoleState.Users : + value === 'members' ? RoleState.Members : + RoleState.Admins); +};
311-328: Consider simplifying the selectedOption logic.The current nested ternary operation for
selectedOptionis hard to read.Consider using an object map:
const stateToOption = { }; // Then in the component: selectedOption={stateToOption[state]}src/screens/OrganizationFundCampaign/OrganizationFundCampagins.tsx (1)
364-382: LGTM! Clean implementation of the sorting functionality.The
SortingButtonimplementation is well-structured with clear sorting options and type-safe value handling. The sorting options cover both funding goal and end date criteria, providing good flexibility for users.Consider extracting the sorting options into a constant to improve maintainability:
const FUND_CAMPAIGN_SORT_OPTIONS = [ { label: t('lowestGoal'), value: 'fundingGoal_ASC' }, { label: t('highestGoal'), value: 'fundingGoal_DESC' }, { label: t('latestEndDate'), value: 'endDate_DESC' }, { label: t('earliestEndDate'), value: 'endDate_ASC' }, ] as const;src/screens/UserPortal/Volunteer/Actions/Actions.tsx (1)
377-399: LGTM! Clean implementation of multiple sorting buttons.The implementation effectively uses two
SortingButtoncomponents to handle both search criteria selection and sorting. The type safety is well-maintained, and the integration with the existing functionality is seamless.Consider extracting the sorting options into constants to improve maintainability:
const SEARCH_CRITERIA_OPTIONS = [ { label: t('assignee'), value: 'assignee' }, { label: t('category'), value: 'category' }, ] as const; const DUE_DATE_SORT_OPTIONS = [ { label: t('latestDueDate'), value: 'dueDate_DESC' }, { label: t('earliestDueDate'), value: 'dueDate_ASC' }, ] as const;src/screens/SubTags/SubTags.tsx (1)
303-312: LGTM! Clean implementation with good integration.The
SortingButtonimplementation effectively handles tag sorting while maintaining type safety withSortedByType. The integration with infinite scroll functionality is well-maintained.Consider these improvements:
- Extract sorting options into a constant:
const TAG_SORT_OPTIONS = [ { label: tCommon('Latest'), value: 'DESCENDING' }, { label: tCommon('Oldest'), value: 'ASCENDING' }, ] as const;
- Consider using lowercase keys for translation consistency:
{ label: tCommon('latest'), value: 'DESCENDING' }, { label: tCommon('oldest'), value: 'ASCENDING' },src/screens/OrganizationTags/OrganizationTags.tsx (1)
317-331: Consider adding aria-label for accessibility.The SortingButton implementation looks good, but consider adding an aria-label to improve accessibility for screen readers.
<SortingButton title="Sort Tags" + aria-label={`Sort tags by ${selectedOption}`} sortingOptions={[ { label: tCommon('Latest'), value: 'latest' }, { label: tCommon('Oldest'), value: 'oldest' }, ]} selectedOption={ tagSortOrder === 'DESCENDING' ? tCommon('Latest') : tCommon('Oldest') } onSortChange={handleSortChange} dataTestIdPrefix="sortTags" className={styles.dropdown} />src/screens/UserPortal/Pledges/Pledges.tsx (1)
411-430: Consider grouping related sort options.The sort options are comprehensive but could be grouped for better user experience.
<SortingButton sortingOptions={[ + // Amount-based sorting { label: t('lowestAmount'), value: 'amount_ASC' }, { label: t('highestAmount'), value: 'amount_DESC' }, + // Date-based sorting { label: t('latestEndDate'), value: 'endDate_DESC' }, { label: t('earliestEndDate'), value: 'endDate_ASC' }, ]} selectedOption={sortBy} onSortChange={(value) => setSortBy( value as | 'amount_ASC' | 'amount_DESC' | 'endDate_ASC' | 'endDate_DESC', ) } dataTestIdPrefix="filter" buttonLabel={tCommon('sort')} + aria-label="Sort pledges" />src/style/app.module.css (2)
271-272: Remove commented code.Commented-out code should be removed rather than left in the codebase as it:
- Creates confusion about whether the code is still needed
- Makes the file harder to maintain
- Adds unnecessary noise to the codebase
Either remove the commented margins or restore them if they're needed:
- /* margin-top: 10px; - margin-bottom: 10px; */
Line range hint
1-1144: Consider adopting a more maintainable CSS architecture.The current CSS structure has several potential issues:
- Large monolithic CSS file that's difficult to maintain
- Global classes that could lead to naming conflicts
- Specificity issues requiring
!importantdirectives- Mixed concerns with styles for multiple components
Consider these improvements:
- Split the CSS into component-specific modules
- Adopt a CSS methodology like BEM or CSS Modules
- Use a CSS-in-JS solution or styled-components for better encapsulation
- Implement a design system with reusable tokens and variables
- Set up CSS linting rules to prevent
!importantusageWould you like me to help create a plan for transitioning to a more maintainable CSS architecture?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
src/components/EventCalendar/EventHeader.spec.tsx(1 hunks)src/components/EventCalendar/EventHeader.tsx(2 hunks)src/components/EventManagement/EventAttendance/EventAttendance.spec.tsx(2 hunks)src/components/EventManagement/EventAttendance/EventAttendance.tsx(4 hunks)src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.spec.tsx(3 hunks)src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.tsx(3 hunks)src/screens/BlockUser/BlockUser.spec.tsx(5 hunks)src/screens/BlockUser/BlockUser.tsx(3 hunks)src/screens/EventVolunteers/Requests/Requests.tsx(3 hunks)src/screens/EventVolunteers/VolunteerGroups/VolunteerGroups.tsx(3 hunks)src/screens/EventVolunteers/Volunteers/Volunteers.spec.tsx(3 hunks)src/screens/EventVolunteers/Volunteers/Volunteers.tsx(4 hunks)src/screens/FundCampaignPledge/FundCampaignPledge.tsx(4 hunks)src/screens/Leaderboard/Leaderboard.spec.tsx(4 hunks)src/screens/Leaderboard/Leaderboard.tsx(3 hunks)src/screens/ManageTag/ManageTag.spec.tsx(2 hunks)src/screens/ManageTag/ManageTag.tsx(2 hunks)src/screens/OrgList/OrgList.test.tsx(2 hunks)src/screens/OrgList/OrgList.tsx(6 hunks)src/screens/OrgPost/OrgPost.test.tsx(1 hunks)src/screens/OrgPost/OrgPost.tsx(2 hunks)src/screens/OrgSettings/OrgSettings.tsx(1 hunks)src/screens/OrganizationActionItems/OrganizationActionItems.spec.tsx(3 hunks)src/screens/OrganizationActionItems/OrganizationActionItems.tsx(4 hunks)src/screens/OrganizationFundCampaign/OrganizationFundCampagins.tsx(3 hunks)src/screens/OrganizationFunds/OrganizationFunds.tsx(3 hunks)src/screens/OrganizationPeople/AddMember.tsx(3 hunks)src/screens/OrganizationPeople/OrganizationPeople.tsx(6 hunks)src/screens/OrganizationTags/OrganizationTags.tsx(3 hunks)src/screens/OrganizationVenues/OrganizationVenues.tsx(3 hunks)src/screens/SubTags/SubTags.spec.tsx(2 hunks)src/screens/SubTags/SubTags.tsx(2 hunks)src/screens/UserPortal/Campaigns/Campaigns.tsx(3 hunks)src/screens/UserPortal/Pledges/Pledges.tsx(3 hunks)src/screens/UserPortal/Volunteer/Actions/Actions.tsx(3 hunks)src/screens/UserPortal/Volunteer/Groups/Groups.tsx(3 hunks)src/screens/UserPortal/Volunteer/Invitations/Invitations.spec.tsx(3 hunks)src/screens/UserPortal/Volunteer/Invitations/Invitations.tsx(4 hunks)src/screens/UserPortal/Volunteer/UpcomingEvents/UpcomingEvents.tsx(5 hunks)src/screens/Users/Users.tsx(3 hunks)src/style/app.module.css(1 hunks)src/subComponents/SortingButton.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (2)
src/screens/OrganizationTags/OrganizationTags.tsx (1)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2387
File: src/screens/OrganizationTags/OrganizationTags.tsx:350-376
Timestamp: 2024-11-12T10:41:04.350Z
Learning: In the `src/screens/OrganizationTags/OrganizationTags.tsx` file, the `OrganizationTags` component uses chunk size with the `InfiniteScroll` component and an outer parent div for handling data loading and scrolling.
src/screens/SubTags/SubTags.tsx (1)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2387
File: src/screens/OrganizationTags/OrganizationTags.tsx:350-376
Timestamp: 2024-11-12T10:41:04.350Z
Learning: In the `src/screens/OrganizationTags/OrganizationTags.tsx` file, the `OrganizationTags` component uses chunk size with the `InfiniteScroll` component and an outer parent div for handling data loading and scrolling.
🔇 Additional comments (40)
src/screens/EventVolunteers/Volunteers/Volunteers.spec.tsx (1)
165-167: Test identifiers simplified for better maintainability.The changes consistently simplify the test identifiers across all status filter test cases, which aligns with the new
SortingButtoncomponent implementation.However, let's verify the test coverage for the new sorting functionality:
Also applies to: 186-188, 207-209
✅ Verification successful
Test identifier changes are valid and well-covered
The simplified test identifiers are consistent with the SortingButton component's implementation, which is extensively used across the application for sorting and filtering. The test coverage adequately verifies the component's functionality through data-testid attributes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for SortingButton component usage # Look for SortingButton component tests echo "Searching for SortingButton component tests..." rg -l "SortingButton" "src/**/*.spec.tsx" "src/**/*.test.tsx" # Look for SortingButton component implementation echo "Searching for SortingButton component implementation..." rg -l "SortingButton" "src/**/*.tsx" --type tsx --glob '!*.spec.tsx' --glob '!*.test.tsx'Length of output: 519
Script:
#!/bin/bash # Find TypeScript/React files and search for SortingButton echo "=== Finding SortingButton implementation and usage ===" fd "\.(tsx|ts)$" src/ | xargs rg "SortingButton" -A 2 -B 2 echo -e "\n=== Finding test files mentioning SortingButton ===" fd ".(spec|test)\.(tsx|ts)$" src/ | xargs rg "SortingButton" -A 2 -B 2Length of output: 32345
src/screens/OrganizationActionItems/OrganizationActionItems.spec.tsx (2)
255-259: Overall test changes look good!The modifications maintain proper test coverage while adapting to the new SortingButton implementation. The test structure remains clean and well-organized.
Also applies to: 272-276, 317-321
255-259: Test IDs have been simplified, but additional test coverage may be needed.The changes to test IDs (
all,pending,completed) are more concise and follow better naming conventions. However, consider the following improvements:
- Add test cases for the new SortingButton component's functionality
- Include edge cases such as:
- Multiple rapid clicks on sorting buttons
- Interaction between sorting and filtering
- Boundary conditions for filtered results
Let's verify the test coverage for the new SortingButton component:
Also applies to: 272-276, 317-321
src/screens/BlockUser/BlockUser.spec.tsx (5)
356-356: LGTM! Test ID update aligns with UI changes.The test ID change from 'showMembers' to 'allMembers' correctly reflects the new UI component implementation.
386-386: Test ID consistently updated across test cases.
462-462: Test ID consistently updated in Last Name Filter test.
511-511: Test ID consistently updated in error handling test.
418-418: Verify test coverage for new sorting functionality.While the test IDs have been updated to match the new UI, we should ensure that the test coverage includes scenarios for the new sorting functionality that replaced the dropdown buttons.
Run this script to check for potential missing test cases:
Also applies to: 425-425
src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.spec.tsx (1)
157-159: LGTM! Test ID changes are consistent.The test ID change from 'statusActive' to 'active' maintains consistency with the new naming pattern.
src/screens/Leaderboard/Leaderboard.spec.tsx (4)
Line range hint
199-203: LGTM! Test ID changes are consistent.The test ID change from 'timeFrameWeekly' to 'weekly' maintains consistency with the new naming pattern.
Line range hint
220-224: LGTM! Test ID changes are consistent.The test ID change from 'timeFrameMonthly' to 'monthly' maintains consistency with the new naming pattern.
Line range hint
241-245: LGTM! Test ID changes are consistent.The test ID change from 'timeFrameYearly' to 'yearly' maintains consistency with the new naming pattern.
Line range hint
176-180: LGTM! Verify corresponding component changes.The test ID change from 'timeFrameAll' to 'allTime' aligns with the new sorting button implementation.
Run this script to verify the component implementation matches the test ID:
✅ Verification successful
Test ID 'allTime' is correctly implemented
The test ID change from 'timeFrameAll' to 'allTime' is properly implemented in both the test and component code. The SortingButton component generates the test ID by combining the prefix 'timeFrame' with the TimeFrame enum value 'allTime'.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the component implementation uses the updated test ID ast-grep --pattern 'data-testid={$_("allTime")}'Length of output: 50
Script:
#!/bin/bash # Search for both old and new test IDs with various syntax patterns echo "=== Searching for new test ID 'allTime' ===" rg "data-testid.*allTime" -A 2 -B 2 echo -e "\n=== Searching for old test ID 'timeFrameAll' ===" rg "data-testid.*timeFrameAll" -A 2 -B 2 echo -e "\n=== Searching with ast-grep for different patterns ===" ast-grep --pattern 'data-testid="allTime"' ast-grep --pattern "data-testid='allTime'" ast-grep --pattern 'dataTestId="allTime"'Length of output: 567
Script:
#!/bin/bash # Broader search for test IDs in tsx files echo "=== Searching for 'allTime' in tsx files ===" rg "allTime" --type tsx -A 2 -B 2 echo -e "\n=== Searching for 'timeFrameAll' in tsx files ===" rg "timeFrameAll" --type tsx -A 2 -B 2 echo -e "\n=== Checking the Leaderboard component specifically ===" fd -e tsx -e ts Leaderboard --exec cat {}Length of output: 23568
src/components/EventCalendar/EventHeader.spec.tsx (1)
72-72: Enhance test coverage for SortingButton integrationThe test only verifies the click event on the new sorting button. Consider adding tests for:
- Different sorting options
- Button label rendering
- Icon rendering based on type
- Accessibility attributes
Let's check current test coverage:
src/screens/OrganizationVenues/OrganizationVenues.tsx (1)
97-99: LGTM! Clean implementation of the search handler.The type casting is properly handled for the searchBy state.
src/screens/UserPortal/Volunteer/Invitations/Invitations.tsx (1)
161-184: LGTM! Clean implementation of sorting and filtering.The SortingButton components are well-implemented with:
- Proper type safety
- Clear separation of sorting and filtering concerns
- Consistent translation usage
src/screens/EventVolunteers/Requests/Requests.tsx (1)
283-294: LGTM! Clean implementation of the sorting functionality.The
SortingButtonimplementation is well-structured with:
- Type-safe sorting options
- Proper state management
- Internationalization support
- Test coverage through data-testid
src/screens/Leaderboard/Leaderboard.tsx (1)
274-298: LGTM! Well-structured implementation of sorting and filtering.The implementation effectively separates sorting and filtering concerns:
- Hours sorting with most/least options
- Time frame filtering with all/weekly/monthly/yearly options
- Type-safe state management
- Clear visual distinction between sorting and filtering through the
typepropsrc/screens/OrganizationFunds/OrganizationFunds.tsx (1)
311-327: LGTM! Clean implementation of the sorting functionality.The
SortingButtoncomponent effectively replaces the previous dropdown implementation for sorting funds by creation date. The component is well-integrated with translations and includes appropriate test IDs.src/screens/EventVolunteers/VolunteerGroups/VolunteerGroups.tsx (1)
335-347: LGTM! Clean implementation of the sorting button.The sorting functionality is well-implemented with clear options and proper type safety.
src/components/EventManagement/EventAttendance/EventAttendance.tsx (1)
196-208: LGTM! Clean implementation of the sorting button.The sorting functionality is well-implemented with clear options and proper typing.
src/screens/EventVolunteers/Volunteers/Volunteers.tsx (1)
336-355: LGTM! Well-structured implementation of sorting and filtering.Both sorting and filtering buttons are well-implemented with:
- Clear separation of concerns
- Proper type safety
- Consistent translation usage
- Descriptive labels and values
Also applies to: 357-368
src/screens/UserPortal/Volunteer/UpcomingEvents/UpcomingEvents.tsx (1)
173-184: LGTM! Well-implemented search criteria toggle.The
SortingButtonimplementation effectively handles the search criteria toggle between title and location. The integration with the search functionality is clean and type-safe.src/screens/ManageTag/ManageTag.spec.tsx (1)
375-375: Verify test IDs match the component implementation.The test assertions have been updated to use 'ASCENDING' and 'DESCENDING' test IDs. Ensure these test IDs match the actual implementation in the ManageTag component.
Also applies to: 377-377, 392-392, 394-394
✅ Verification successful
Test IDs are correctly implemented and consistent across components
The test IDs 'ASCENDING' and 'DESCENDING' are part of a shared sorting pattern used consistently across multiple components (ManageTag, SubTags). The implementation matches the test assertions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the corresponding test IDs in the component implementation ast-grep --pattern 'data-testid="ASCENDING"' ast-grep --pattern 'data-testid="DESCENDING"'Length of output: 94
Script:
#!/bin/bash # Find the component file fd ManageTag.tsx # Search for test IDs with ripgrep (case insensitive, including partial matches) rg -i "testid.*ascend" --type tsx rg -i "testid.*descend" --type tsx # Search with more flexible ast-grep patterns ast-grep --pattern 'data-testid=$_'Length of output: 221
Script:
#!/bin/bash # Search in the specific component file with context rg -i "testid.*" src/screens/ManageTag/ManageTag.tsx -B 2 -A 2 # Also check the test file for context rg -i "testid.*" src/screens/ManageTag/ManageTag.spec.tsx -B 2 -A 2Length of output: 11593
Script:
#!/bin/bash # Search for SortDropdown component fd SortDropdown # Search for the implementation of sort-related test IDs rg -i "testid.*ascend" --type-add 'tsx:*.{tsx,ts}' rg -i "testid.*descend" --type-add 'tsx:*.{tsx,ts}'Length of output: 924
src/screens/OrganizationTags/OrganizationTags.tsx (1)
295-297: LGTM! Clean and concise implementation.The
handleSortChangefunction provides a clear mapping between the sorting button values and the internal sort order state.src/screens/Users/Users.tsx (1)
375-397: LGTM! Well-structured implementation with proper type safety.The SortingButton components are well-implemented with:
- Clear sorting and filtering options
- Proper use of translations
- Type-safe value handling
src/screens/UserPortal/Pledges/Pledges.tsx (1)
398-409: LGTM! Clean implementation of search criteria selection.The SortingButton implementation for search criteria is well-structured with:
- Clear options for searching by pledgers or campaigns
- Proper type casting in the onChange handler
- Good use of translations
src/screens/OrgList/OrgList.test.tsx (1)
Line range hint
526-538: LGTM! Test coverage for sorting functionality looks good.The test cases properly verify the sorting functionality by:
- Testing the "Latest" sorting option
- Testing the "Earliest" sorting option
- Using appropriate data-testid attributes for reliable element selection
src/screens/OrgPost/OrgPost.tsx (1)
21-21: LGTM! Clean implementation of SortingButton components.The replacement of dropdowns with SortingButton components improves consistency and maintainability. The implementation:
- Properly handles search type selection (Text/Title)
- Manages post sorting (Latest/Oldest)
- Uses clear data test IDs for testing
- Maintains proper translation support
Also applies to: 305-329
src/screens/OrgList/OrgList.tsx (2)
315-320: LGTM! Improved sorting state management.The handleSortChange function properly updates the sorting state and triggers a refetch with the correct ordering parameters.
354-363: LGTM! Well-structured SortingButton implementation.The SortingButton implementation:
- Provides clear sorting options (Latest/Earliest)
- Maintains proper state management
- Uses appropriate data test IDs
- Follows consistent translation patterns
src/screens/OrganizationActionItems/OrganizationActionItems.tsx (4)
139-142: LGTM! Proper data refetching implementation.The useEffect hook correctly triggers a refetch when sorting or status filters change, ensuring the UI stays in sync with the selected filters.
391-404: LGTM! Well-implemented search type selection.The SortingButton for search type selection:
- Properly handles assignee/category options
- Maintains type safety with proper TypeScript casting
- Uses appropriate translations
405-420: LGTM! Clean implementation of sort by due date.The SortingButton for due date sorting:
- Handles both ascending and descending orders
- Uses clear labels for sorting options
- Maintains proper state management
421-441: LGTM! Well-structured status filtering.The SortingButton for status filtering:
- Provides all necessary status options (all/pending/completed)
- Properly handles null state for "all" option
- Uses consistent translation patterns
src/screens/FundCampaignPledge/FundCampaignPledge.tsx (1)
498-517: Well-implemented sorting functionality!The
SortingButtonimplementation is clean and follows best practices:
- Clear sorting options with proper translations
- Type-safe implementation with explicit type casting
- Proper test coverage support with data test IDs
src/screens/OrganizationPeople/AddMember.tsx (2)
277-283: Clean handler implementation!The
handleSortChangefunction is well-implemented with clear conditional logic for handling different sorting options.
287-297: Well-structured SortingButton implementation!The component is properly configured with:
- Clear sorting options using translations
- Proper event handling
- Styling support via className
src/screens/ManageTag/ManageTag.tsx (1)
380-392: Clean SortingButton implementation!The component is well-configured with:
- Proper translations for sorting options
- Type-safe implementation with SortedByType
- Test coverage support with data test IDs
src/screens/OrgPost/OrgPost.test.tsx (1)
311-311: Test ID correctly updated!The test ID change aligns with the new SortingButton implementation while maintaining the test's functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/EventCalendar/EventHeader.tsx (1)
79-90:⚠️ Potential issueImplement proper event type handler.
The
onSortChangehandler for event type selection is incomplete, only logging to console.Consider implementing proper state management and filtering logic for event types. This could involve:
- Adding state to track selected event type
- Implementing filtering logic based on the selection
- Updating the UI to reflect the filtered results
-onSortChange={(value) => console.log(`Selected: ${value}`)} +onSortChange={(value) => { + setEventType(value); + filterEventsByType(value); +}}
🧹 Nitpick comments (1)
src/screens/OrgList/OrgList.spec.tsx (1)
528-528: LGTM! Test IDs updated to match new UI labels.The test ID changes from 'latest'/'oldest' to 'Latest'/'Earliest' align with the new sorting button implementation.
Consider enhancing test coverage.
Since this is part of a larger refactoring to replace dropdown/filter buttons with a customized sorting button, consider adding test cases to verify:
- The new SortingButton component's rendering
- Interaction with sorting options
- Accessibility of the sorting interface
Also applies to: 540-540
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/EventCalendar/EventHeader.tsx(2 hunks)src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.tsx(3 hunks)src/screens/OrgList/OrgList.spec.tsx(2 hunks)src/screens/OrgList/OrgList.tsx(6 hunks)src/screens/OrganizationFunds/OrganizationFunds.tsx(3 hunks)src/screens/OrganizationPeople/AddMember.tsx(3 hunks)src/screens/OrganizationPeople/OrganizationPeople.tsx(6 hunks)src/screens/OrganizationVenues/OrganizationVenues.tsx(3 hunks)src/style/app.module.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/screens/OrgList/OrgList.tsx
- src/screens/OrganizationPeople/AddMember.tsx
- src/style/app.module.css
🔇 Additional comments (5)
src/components/EventCalendar/EventHeader.tsx (1)
67-78: LGTM!The view type sorting button implementation is complete and properly integrated with the existing
handleChangeViewfunction.src/screens/OrganizationVenues/OrganizationVenues.tsx (1)
165-189: LGTM!The implementation of both sorting buttons is complete and well-integrated:
- Search criteria button properly updates the search filter
- Sort button correctly manages the capacity-based sorting
- Type safety is maintained with proper type assertions
src/screens/OrganizationFunds/OrganizationFunds.tsx (1)
311-327: LGTM!The sorting button implementation is complete and well-integrated:
- Properly manages sorting by creation date
- Maintains type safety with appropriate type assertions
- Correctly displays the selected option
src/components/OrgSettings/ActionItemCategories/OrgActionItemCategories.tsx (1)
309-347: LGTM!Both sorting buttons are well-implemented:
- Sort button properly manages creation date sorting
- Status filter button correctly handles all status options
- Type safety is maintained throughout
- Selected options are displayed correctly
src/screens/OrganizationPeople/OrganizationPeople.tsx (1)
313-315: LGTM!The sorting button implementation is complete and well-integrated:
- Properly manages role-based filtering
- Correctly updates state and triggers data fetching
- Selected option display is handled appropriately
Also applies to: 345-362
|
@palisadoes Why this linting action failing because of these CSS errors? I didn't update those. |
|
#3041, the PR that is creating the problem |
|
Please fix the failing tests |
|
@palisadoes This action is causing the issue; this action was added recently and needs some updating.
|
|
I reverted the PR. Please make a minor commit to reset the testing workflow |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/screens/OrgList/OrgList.tsx (2)
315-320: Consider extracting sorting constants and reducing duplicationThe sorting logic works correctly but could be improved for maintainability.
Consider these improvements:
+ // Add at the top of the file with other constants + const SORT_OPTIONS = { + LATEST: 'Latest', + EARLIEST: 'Earliest', + } as const; + + const getSortOrderBy = (option: string): string => + option === SORT_OPTIONS.LATEST ? 'createdAt_DESC' : 'createdAt_ASC'; const handleSortChange = (value: string): void => { setSortingState({ option: value, selectedOption: t(value), }); - const orderBy = value === 'Latest' ? 'createdAt_DESC' : 'createdAt_ASC'; + const orderBy = getSortOrderBy(value); refetchOrgs({ first: perPageResult, skip: 0, filter: searchByName, orderBy, }); };
354-364: Consider localizing the title propThe SortingButton implementation looks good, but the title could be improved.
Consider using the translation function for the title:
<SortingButton - title="Sort organizations" + title={t('sortOrganizationsTitle')} sortingOptions={[ { label: t('Latest'), value: 'Latest' }, { label: t('Earliest'), value: 'Earliest' }, ]} selectedOption={sortingState.selectedOption} onSortChange={handleSortChange} dataTestIdPrefix="sortOrgs" dropdownTestId="sort" />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/screens/ManageTag/ManageTag.spec.tsx(2 hunks)src/screens/OrgList/OrgList.tsx(6 hunks)src/screens/OrgSettings/OrgSettings.spec.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/ManageTag/ManageTag.spec.tsx
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
src/screens/OrgSettings/OrgSettings.spec.tsx
[warning] 133-133:
Some tests seem to be commented
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (3)
src/screens/OrgSettings/OrgSettings.spec.tsx (1)
133-178: Add tests for the new SortingButton functionality.The dropdown functionality has been replaced with a new SortingButton component, but there are no tests covering this new functionality. Please add test cases to verify:
- Rendering of the SortingButton
- Interaction with the SortingButton
- Correct behavior when sorting options are selected
Would you like me to help generate test cases for the new SortingButton functionality?
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 133-133:
Some tests seem to be commentedsrc/screens/OrgList/OrgList.tsx (2)
31-31: LGTM! Clean integration of SortingButton componentThe import is well-organized and the state management follows React best practices with clear type definitions.
Also applies to: 60-63
Line range hint
208-212: Verify error handling strategyThe current error handling strategy might be too aggressive by clearing localStorage and redirecting to home for any GraphQL error.
Consider implementing more granular error handling based on error types. Let's verify the impact:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/subComponents/SortingButton.tsx (5)
15-34: Consider refining prop types for better type safety.The interface definitions could be improved:
dataTestIdPrefixis marked as required but could be optional since it's only used for testing.classNamehas a default value but isn't marked as optional in the interface.interface InterfaceSortingButtonProps { // ...other props - dataTestIdPrefix: string; + dataTestIdPrefix?: string; // ...other props - className: string; + className?: string; // ...remaining props }
68-79: Enhance accessibility for dropdown menu.The dropdown menu lacks proper ARIA attributes for accessibility. Consider adding:
aria-labelfor the menurole="menuitem"for menu items- <Dropdown.Menu> + <Dropdown.Menu aria-label={`${title || 'Sort'} options`}> {sortingOptions.map((option) => ( <Dropdown.Item key={option.value} onClick={() => onSortChange(option.value)} data-testid={`${option.value}`} className={styles.dropdownItem} + role="menuitem" >
59-67: Simplify variant logic and handle edge cases.The variant logic can be simplified, and we should handle the case where both
buttonLabelandselectedOptionare undefined.<Dropdown.Toggle - variant={selectedOption === '' ? 'outline-success' : 'success'} + variant={!selectedOption ? 'outline-success' : 'success'} data-testid={`${dataTestIdPrefix}`} className={className} > <IconComponent className={'me-1'} /> - {buttonLabel || selectedOption} + {buttonLabel || selectedOption || 'Sort'}
84-98: Complete PropTypes validation.Add missing PropTypes and consider defining defaultProps:
SortingButton.propTypes = { // ... existing props + className: PropTypes.string, }; +SortingButton.defaultProps = { + type: 'sort', + className: styles.dropdown, + title: '', + selectedOption: '', + buttonLabel: '', +};
43-82: Consider performance optimization.For better performance, consider memoizing the component using
React.memoto prevent unnecessary re-renders when parent components update with the same props.-const SortingButton: React.FC<InterfaceSortingButtonProps> = ({ +const SortingButton: React.FC<InterfaceSortingButtonProps> = React.memo(({ // ... props -}); +}));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/subComponents/SortingButton.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
src/subComponents/SortingButton.tsx (1)
58-58: Fix accessibility attribute usage.The
aria-expandedattribute should be dynamically bound to the Dropdown's state.
|
@palisadoes PTAL |
palisadoes
merged commit ea7851d
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
refactoring
Issue Number:
Fixes #3224
Did you add tests for your changes?
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
Release Notes
New Features
SortingButtoncomponent across multiple screens to replace dropdown menus.Improvements
Bug Fixes
Technical Updates
Style Changes
These release notes capture the high-level changes while maintaining confidentiality about specific implementation details.