-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Ability to Filter by Creation Source #7078
base: main
Are you sure you want to change the base?
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.
PR Summary
This pull request implements the ability to filter records by Creation Source, addressing issue #6808. Key changes include:
- Added 'SOURCE' filter type in FilterType.ts and related components
- Implemented sub-menu functionality for 'ACTOR' filter type in ObjectFilterDropdownFilterSelect.tsx
- Created new ObjectFilterDropdownSourceSelect.tsx component for source filtering
- Updated turnObjectDropdownFilterIntoQueryFilter.ts to handle 'SOURCE' subfield in ACTOR filters
- Modified FilterDefinition type to include optional 'subFieldType' for more granular filtering
- Added utility functions getSubMenuOptions.ts and hasSubMenuFilter.ts to support new filtering structure
15 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
{getSubMenuOptions(currentSubMenu) | ||
.sort((a, b) => a.name.localeCompare(b.name)) | ||
.filter((item) => | ||
item.name | ||
.toLocaleLowerCase() | ||
.includes(searchText.toLocaleLowerCase()), | ||
) |
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.
style: Consider memoizing this filtered and sorted array to optimize performance
[rawUIFilter.definition.subFieldType.toLowerCase()]: { | ||
in: parsedRecordIds, | ||
} as RelationFilter, |
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.
logic: Dynamic subfield type access might cause issues if subFieldType is unexpected
</SelectableList> | ||
{!currentSubMenu ? ( | ||
<> | ||
<StyledInput |
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.
Could you please extract this into a component because this ternary is quite heavy to read
import { useSelectableList } from '@/ui/layout/selectable-list/hooks/useSelectableList'; | ||
import { MenuItemSelect } from '@/ui/navigation/menu-item/components/MenuItemSelect'; | ||
import { useRecoilValue } from 'recoil'; | ||
import { useIcons } from 'twenty-ui'; | ||
|
||
export type ObjectFilterDropdownFilterSelectMenuItemProps = { | ||
filterDefinition: FilterDefinition; | ||
setCurrentSubMenu: (subMenu: FilterType) => void; |
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.
Avoid passing setters in props, it's an anti-pattern, use recoil states instead.
const selectedFilter = useRecoilValue(selectedFilterState); | ||
|
||
// Dynamically update sourceEnumOptions based on selectedRecordIds | ||
const sourceEnumOptions: SelectableRecord[] = [ |
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.
Could you extract this into a util with proper self-explanatory naming ?
|
||
export const ObjectFilterSelectSubMenu = ({ | ||
currentSubMenu, | ||
setCurrentSubMenu, |
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.
No setter in props
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.
This should be a recoil state
}: ObjectFilterSelectSubMenuProps) => { | ||
const [searchText, setSearchText] = useState(''); | ||
|
||
const getHeaderTitle = (subMenu: FilterType | null) => { |
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.
Extract this into its own file
@@ -0,0 +1,3 @@ | |||
import { FilterType } from '@/object-record/object-filter-dropdown/types/FilterType'; | |||
|
|||
export const hasSubMenuFilter = (type: FilterType) => ['ACTOR'].includes(type); |
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.
type === 'ACTOR' ?
import { ViewFilter } from '../types/ViewFilter'; | ||
|
||
const getDefinitionForFilter = ( |
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.
Extract this into a util and rename getFilterDefinitionForViewFilter
Thanks for the review @lucasbordeau , We are working on it |
This PR was created by GitStart to address the requirements from this ticket: TWNTY-6808.
This ticket was imported from: TWNTY-6808
Description
Demo
LOOM: https://www.loom.com/share/dba9c3d37a4242fe90f977b1babffbde?sid=59b07c51-d245-43cc-bb38-7d898ef72878
Refs
#6808
Fixes #6808