-
Notifications
You must be signed in to change notification settings - Fork 19
Dynamic filter #231
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
Dynamic filter #231
Conversation
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 think if we did not disable poly search and do validating, lets finish it so it will be universal (might happen indeed)
: [col.foreignResource.searchableFields]; | ||
|
||
searchableFields.forEach((fieldName) => { | ||
if (typeof fieldName !== 'string') { |
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.
@SerVitasik or text at least
const targetResource = this.inputConfig.resources.find((r) => r.resourceId === pr.resourceId || r.table === pr.resourceId); | ||
if (targetResource) { | ||
const hasField = targetResource.columns.some((targetCol) => targetCol.name === fieldName); | ||
if (!hasField) { |
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.
so if at least one resource will not have column it will crash. This should be changed to - if there are no resources which have this column then crash.
Probably a little bit implicit but i suggest next behaviour:
if poly has 2 resources:
- if res A has column C and res be has no col C - then when searching use column C in where of res A query, but dont use in query of res B (in query for res b use columns which exist)
- if both res A and B has col C - use it in both queries (to both resources)
- if no of resources has column C - drop this error here and say explicitly - we can't find this column neither in A nor in B.
So when applying poly search (later in code) - extract subset of this searchable fields which exist in resource and use only them in search with disjunction way
@@ -905,6 +905,34 @@ export default class AdminForthRestAPI implements IAdminForthRestAPI { | |||
throw new Error(`Wrong filter object value: ${JSON.stringify(filters)}`); | |||
} | |||
} | |||
|
|||
if (search && search.trim() && columnConfig.foreignResource.searchableFields) { |
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 think poly case is not implemented here at all - it should do parallel queries
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.
or probably you can do it on frontend (later) - should be possible also, maybe even better
...acc, | ||
[c.name]: debounce((searchTerm) => { | ||
searchOptions(c.name, searchTerm); | ||
}, c.filterOptions?.debounceTimeMs || 300), |
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.
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.
c.filterOptions?.debounceTimeMs i am already using this.
You told me that we have constant 300 but i didn't find it
...acc, | ||
[c.name]: debounce((searchTerm: string) => { | ||
searchOptions(c.name, searchTerm); | ||
}, 300), |
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.
@SerVitasik same here
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.
Here i can't use c.filterOptions?.debounceTimeMs because this is only for filters
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.
Pull Request Overview
This PR introduces dynamic filtering capabilities for foreign resource relationships, enabling search functionality in dropdown components and pagination for large datasets. The implementation adds support for searching within foreign resource fields and handling polymorphic resource relationships.
- Adds searchable fields configuration for foreign resources with case sensitivity options
- Implements debounced search functionality with pagination support for dropdown components
- Refactors existing static foreign resource loading to dynamic loading with scroll-based pagination
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
dev-demo/resources/clinics.ts | Updates data source reference and enables UUID generation |
dev-demo/resources/api_keys.ts | Adds searchable fields configuration to foreign resource |
adminforth/types/Common.ts | Defines new type interfaces for searchable fields and case sensitivity |
adminforth/spa/src/utils.ts | Implements core utility functions for pagination, search, and debouncing |
adminforth/spa/src/components/ResourceForm.vue | Refactors from static to dynamic foreign resource loading |
adminforth/spa/src/components/Filters.vue | Updates filter components to support dynamic search and pagination |
adminforth/spa/src/components/ColumnValueInputWrapper.vue | Adds unmasked prop passing |
adminforth/spa/src/components/ColumnValueInput.vue | Integrates search and pagination functionality |
adminforth/spa/src/afcl/Select.vue | Enhances select component with search disable and scroll events |
adminforth/modules/restApi.ts | Implements server-side search filtering for foreign resources |
adminforth/modules/configValidator.ts | Adds validation for searchable fields configuration |
adminforth/documentation/ | Updates documentation with searchable fields examples |
adminforth/dataConnectors/baseConnector.ts | Adds polymorphic resource field validation handling |
Comments suppressed due to low confidence (1)
adminforth/spa/src/utils.ts:10
- The 'debounce' package import should be verified. Consider using a more established package like 'lodash.debounce' or implementing a simple debounce function locally to avoid adding external dependencies.
import debounce from 'debounce';
body: { | ||
resourceId, | ||
column: columnName, | ||
limit: 100, |
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 hardcoded limit value of 100 should use the ITEMS_PER_PAGE_LIMIT constant defined at the top of the file for consistency.
limit: 100, | |
limit: ITEMS_PER_PAGE_LIMIT, |
Copilot uses AI. Check for mistakes.
} | ||
columnOptions.value[columnName].push(...list.items); | ||
|
||
columnOffsets[columnName] += 100; |
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 hardcoded offset increment of 100 should use the ITEMS_PER_PAGE_LIMIT constant for consistency.
Copilot uses AI. Check for mistakes.
body: { | ||
resourceId, | ||
column: columnName, | ||
limit: 100, |
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 hardcoded limit value of 100 should use the ITEMS_PER_PAGE_LIMIT constant for consistency.
Copilot uses AI. Check for mistakes.
columnOptions.value = {}; | ||
} | ||
columnOptions.value[columnName] = list.items; | ||
columnOffsets[columnName] = 100; |
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 hardcoded offset value of 100 should use the ITEMS_PER_PAGE_LIMIT constant for consistency.
columnOffsets[columnName] = 100; | |
columnOffsets[columnName] = ITEMS_PER_PAGE_LIMIT; |
Copilot uses AI. Check for mistakes.
import { computedAsync } from '@vueuse/core'; | ||
import { computed, onMounted, ref, watch } from 'vue'; | ||
import { computed, onMounted, reactive, ref, watch, provide } from 'vue'; |
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.
[nitpick] The import statement is quite long. Consider breaking it into multiple lines for better readability.
import { computed, onMounted, reactive, ref, watch, provide } from 'vue'; | |
import { | |
computed, | |
onMounted, | |
reactive, | |
ref, | |
watch, | |
provide | |
} from 'vue'; |
Copilot uses AI. Check for mistakes.
@@ -38,8 +38,9 @@ | |||
</div> | |||
<teleport to="body" v-if="teleportToBody && showDropdown"> | |||
<div ref="dropdownEl" :style="getDropdownPosition" :class="{'shadow-none': isTop}" | |||
class="fixed z-[5] w-full bg-lightDropdownOptionsBackground shadow-lg dark:shadow-black dark:bg-darkDropdownOptionsBackground | |||
dark:border-gray-600 rounded-md py-1 text-base ring-1 ring-black ring-opacity-5 overflow-auto focus:outline-none sm:text-sm max-h-48"> | |||
class="fixed z-[5] w-full bg-white shadow-lg dark:shadow-black dark:bg-gray-700 |
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 hardcoded colors 'bg-white' and 'dark:bg-gray-700' replace the original theme-aware classes. This could break theming consistency. Consider preserving the original 'bg-lightDropdownOptionsBackground' and 'dark:bg-darkDropdownOptionsBackground' classes.
class="fixed z-[5] w-full bg-white shadow-lg dark:shadow-black dark:bg-gray-700 | |
class="fixed z-[5] w-full bg-lightDropdownOptionsBackground shadow-lg dark:shadow-black dark:bg-darkDropdownOptionsBackground |
Copilot uses AI. Check for mistakes.
class="absolute z-10 mt-1 w-full bg-white shadow-lg dark:shadow-black dark:bg-gray-700 | ||
dark:border-gray-600 rounded-md py-1 text-base ring-1 ring-black ring-opacity-5 overflow-auto focus:outline-none sm:text-sm max-h-48" |
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.
Same theming issue as the teleported dropdown. The hardcoded colors should be replaced with the original theme-aware classes for consistency.
class="absolute z-10 mt-1 w-full bg-white shadow-lg dark:shadow-black dark:bg-gray-700 | |
dark:border-gray-600 rounded-md py-1 text-base ring-1 ring-black ring-opacity-5 overflow-auto focus:outline-none sm:text-sm max-h-48" | |
class="absolute z-10 mt-1 w-full bg-lightDropdownOptionsBackground shadow-lg dark:shadow-black dark:bg-darkDropdownOptionsBackground | |
dark:border-darkDropdownOptionsBorder rounded-md py-1 text-base ring-1 ring-black ring-opacity-5 overflow-auto focus:outline-none sm:text-sm max-h-48" |
Copilot uses AI. Check for mistakes.
No description provided.