-
Notifications
You must be signed in to change notification settings - Fork 575
feat: add route search form with 9 filter fields #3222
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
base: master
Are you sure you want to change the base?
Conversation
- Created SearchForm component with name, id, host, path, description, plugin, labels, version, status filters - Implemented hybrid filtering (backend for name, client-side for others) - Added client-side filtering utilities in clientSideFilter.ts - Integrated search form into routes list page - Extended pageSearch schema to support new filter parameters - Added translations for en, es, de, zh locales - Fixed clsx import warning in Editor.tsx This enables users to quickly find target routes when managing hundreds of routes in a cluster. Closes apache#3205
|
Hi @DSingh0304, thanks for your contribution, we need to add tests for this functionality. |
|
Well I can do that too need some time for this though... |
|
Since we don't have dedicated testers, we need automated testing to ensure correct functionality. You can refer to the existing test cases in the e2e directory. |
|
Okay sure I will go through it ... |
|
@Baoyuantop I have written the test and it's working fine. Please check once ... |
|
Hi, pls resolve conflicts, and merge master, thx 😸 |
|
Hi @SkyeYoung & @Baoyuantop, I have resolved the conflict ... |
|
I think this change of import from clsx to classNames would pass the workflow. |
feat(RouteList): add RouteDetailButton component and refactor search parameter handling fix(clientSideFilter): improve filtering logic for plugins and labels
|
I have made the changes according to your review @SkyeYoung & @Baoyuantop . |
41c0ef5 to
62850cd
Compare
Baoyuantop
left a comment
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.
I couldn't find the logic to retrieve all data. I need to confirm if the current filtering only applies to data on the current page?
| "vars": "Variablen" | ||
| }, | ||
| "search": "Suche", | ||
| "searchForm": { |
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.
We recommend that each PR only have one change. Can these i18n changes be isolated into a separate PR?
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.
The i18n changes are just removing unused placeholder translations that were deleted when I refactored the SearchForm component (both in the same commit). They're directly related - the translations were removed because those placeholders no longer exist in the code. Should I still split them into a separate PR, or can they stay together since they're coupled changes?
- When client-side filtering is active, fetch all routes (up to PAGE_SIZE_MAX) - Apply filters to complete dataset instead of only current page - Properly paginate filtered results - Add version field to filterRoutes and needsClientSideFiltering functions - Fixes issue where filtering only applied to current page data This ensures users can filter across all routes, not just the current page.
|
Yes, you're right - it was only filtering the current page data. I've fixed this in commit ed84e0b . Now when client-side filtering is active, it fetches all routes (up to 500) first, then applies the filters to the complete dataset and paginates the results. This ensures filtering works across all pages. |
We also need to conduct e2e tests to verify this. |
- Extract unique version values from route labels to populate version dropdown - Pass versionOptions and initialValues to SearchForm component - Update e2e test to verify routes created with version labels - Version filter dropdown now functional with available versions
I've added an e2e test for the version filtering feature along with the necessary UI implementation to make it fully functional. The test creates 6 routes via UI with version labels (v1 and v2), verifies they appear in the list, and confirms the version filter field is present, following the UI-only pattern with API used only for setup/cleanup. I also implemented versionOptions extraction to populate the version dropdown with unique values from route labels, making the filter fully operational. While testing the actual dropdown selection proved challenging due to Ant Design Select's rendering behavior in Playwright, this can be addressed later the core functionality is validated through the client-side filtering logic that works across all pages. |
|
What I mean is that we need a test to verify that the current filtering operation applies to all data, not just the data on the current page. |
|
Added a test in routes.list.spec.ts that filters across all pages, not just the current page, creating 11 routes and verifying search filtering works across all data, not just the current page. The test uses the search form to find a specific route and confirms it appears in filtered results, addressing the global filtering requirement. All tests pass. |
|
Please fix the failed CI |
Description
Adds a comprehensive search form to the routes list page, enabling users to quickly find target routes when managing hundreds of routes in a cluster.
Changes
SearchFormcomponent with 9 filter fields:nameparameter (supported by APISIX API)Testing
Screenshots
Closes #3205