Reorganized components in advanced filter dropdown #11089
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lterRecordFilterRow - Abstracted advanced filter record filter group components into AdvancedFilterRecordFilterGroupRow - Moved advanced filter field selection components in AdvancedFilterFieldSelectDrodownButton and AdvancedFilterFieldSelectDrodownContent - Moved advanced filter field selection search input into AdvancedFilterFieldSelectSearchInput - Created AdvancedFilterDropdownRow UI component - Renamed view filter to record filter in modified files
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR reorganizes the advanced filter dropdown components to improve code clarity and maintainability, particularly focusing on recursive filter structures and component naming conventions.
- Introduced new components like
AdvancedFilterRecordFilterGroupRowandAdvancedFilterRecordFilterRowto clarify recursive filter hierarchy - Abstracted shared UI elements into standalone components (e.g.,
AdvancedFilterDropdownRow,AdvancedFilterFieldSelectSearchInput) - Renamed components from "ViewFilter" to "RecordFilter" for consistency across the codebase
- Separated advanced filter code path from view bar simple filter code path while maintaining reusability
- Fixed typo in component naming ('Drodown' instead of 'Dropdown') in
AdvancedFilterFieldSelectDrodownButton
15 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
| import { objectFilterDropdownIsSelectingCompositeFieldComponentState } from '@/object-record/object-filter-dropdown/states/objectFilterDropdownIsSelectingCompositeFieldComponentState'; | ||
| import { useRecoilComponentStateV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentStateV2'; | ||
|
|
||
| export const AdvancedFilterFieldSelectDrodownContent = () => { |
There was a problem hiding this comment.
syntax: Typo in component name: 'Drodown' should be 'Dropdown'
Suggested change
| export const AdvancedFilterFieldSelectDrodownContent = () => { | |
| export const AdvancedFilterFieldSelectDropdownContent = () => { |
| @@ -0,0 +1,53 @@ | |||
| import { AdvancedFilterFieldSelectDrodownContent } from '@/object-record/advanced-filter/components/AdvancedFilterFieldSelectDrodownContent'; | |||
There was a problem hiding this comment.
syntax: 'Drodown' is misspelled in the import path and component name. Should be 'Dropdown'
Comment on lines
+8
to
+10
| border: none; | ||
| border-top: none; | ||
| border-bottom: 1px solid ${({ theme }) => theme.border.color.light}; |
There was a problem hiding this comment.
style: Redundant border declarations. border: none already sets all borders to none, making border-top: none unnecessary.
Suggested change
| border: none; | |
| border-top: none; | |
| border-bottom: 1px solid ${({ theme }) => theme.border.color.light}; | |
| border: none; | |
| border-bottom: 1px solid ${({ theme }) => theme.border.color.light}; |
| export const AdvancedFilterRecordFilterOperandSelect = ({ | ||
| recordFilterId, | ||
| }: AdvancedFilterRecordFilterOperandSelectProps) => { | ||
| const dropdownId = `advanced-filter-view-filter-operand-${recordFilterId}`; |
There was a problem hiding this comment.
syntax: dropdownId still contains 'view-filter' in the string template despite the component being renamed to use 'record-filter'
| flex-direction: column; | ||
| gap: ${({ theme }) => theme.spacing(2)}; | ||
| padding: ${({ theme }) => theme.spacing(2)}; | ||
| overflow: hidden; |
There was a problem hiding this comment.
style: overflow: hidden may clip dropdown menus in nested filter groups
| @@ -0,0 +1,44 @@ | |||
| import { AdvancedFilterDropdownRow } from '@/object-record/advanced-filter/components/AdvancedFilterDropdownRow'; | |||
| import { AdvancedFilterFieldSelectDrodownButton } from '@/object-record/advanced-filter/components/AdvancedFilterFieldSelectDrodownButton'; | |||
There was a problem hiding this comment.
syntax: Typo in import name 'AdvancedFilterFieldSelectDrodownButton' (missing 'w' in 'Dropdown')
Suggested change
| import { AdvancedFilterFieldSelectDrodownButton } from '@/object-record/advanced-filter/components/AdvancedFilterFieldSelectDrodownButton'; | |
| import { AdvancedFilterFieldSelectDropdownButton } from '@/object-record/advanced-filter/components/AdvancedFilterFieldSelectDropdownButton'; |
Comment on lines
+21
to
+23
| <ObjectFilterDropdownComponentInstanceContext.Provider | ||
| value={{ instanceId: `advanced-filter-${recordFilter.id}` }} | ||
| > |
There was a problem hiding this comment.
style: Consider memoizing the context value to prevent unnecessary re-renders of child components
| export const AdvancedFilterValueInputDropdownButton = ({ | ||
| recordFilterId, | ||
| }: AdvancedFilterValueInputDropdownButtonProps) => { | ||
| const dropdownId = `advanced-filter-view-filter-value-input-${recordFilterId}`; |
There was a problem hiding this comment.
syntax: dropdownId still contains 'view-filter' in string despite renaming to recordFilter
Suggested change
| const dropdownId = `advanced-filter-view-filter-value-input-${recordFilterId}`; | |
| const dropdownId = `advanced-filter-record-filter-value-input-${recordFilterId}`; |
| clickableComponent={ | ||
| <StyledValueDropdownContainer> | ||
| {operandHasNoInput ? ( | ||
| <></> |
There was a problem hiding this comment.
style: consider using null instead of empty fragment for consistency with other null returns in the codebase
| import { DropdownMenuItemsContainer } from '@/ui/layout/dropdown/components/DropdownMenuItemsContainer'; | ||
| import { useRecoilComponentStateV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentStateV2'; | ||
| import { useRecoilComponentValueV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentValueV2'; | ||
| import { useSetRecoilComponentStateV2 } from '@/ui/utilities/state/component-state/hooks/useSetRecoilComponentStateV2'; | ||
| import { useState } from 'react'; | ||
| import { isDefined } from 'twenty-shared'; | ||
| import { IconApps, IconChevronLeft, MenuItem, useIcons } from 'twenty-ui'; | ||
| import { DropdownMenuHeaderLeftComponent } from '@/ui/layout/dropdown/components/DropdownMenuHeader/internal/DropdownMenuHeaderLeftComponent'; | ||
|
|
||
| export const ObjectFilterDropdownFilterSelectCompositeFieldSubMenu = () => { | ||
| const [searchText] = useState(''); |
There was a problem hiding this comment.
logic: searchText state is defined but never used since the search input is commented out
| margin: 0; | ||
| outline: none; | ||
| padding: ${({ theme }) => theme.spacing(2)}; | ||
| min-height: 19px; |
There was a problem hiding this comment.
looks wrong but I guess it was already there!
There was a problem hiding this comment.
Indeed. Let's see if I can tackle it in another PR !
charlesBochet
approved these changes
Mar 21, 2025
ijreilly
pushed a commit
that referenced
this pull request
Mar 24, 2025
This PR essentially focuses on a refactor of the component hierarchy and naming in advanced filter dropdown, to make it more readable and easy to maintain. This refactor was required because this area of the code is recursive, so it's better to see the same abstract components in the recursion, instead of trying to guess whether we have the same components than the level above or not. Also keep in mind that this refactor is meant to separate the advanced filter code path from the view bar simple filter code path, while reusing what's reusable, so here we have a first attempt at finding the sweet spot, that we'll be able to duplicate on other filter dropdown use cases. - We now use AdvancedFilterRecordFilterGroupRow and AdvancedFilterRecordFilterRow to make it clearer in the advanced filter dropdown recursion where we are. - Children component of AdvancedFilterRecordFilterRow have been abstracted at the same level to make reading easier - The field selection dropdown is now in a self-explanatory component that follows the same naming pattern as other dropdowns in the app : AdvancedFilterFieldSelectDrodownButton, together with AdvancedFilterFieldSelectDrodownContent. - The field selection search in the filter dropdown is now a standalone component : AdvancedFilterFieldSelectSearchInput - The UI container of a row has been abstracted in a new AdvancedFilterDropdownRow Miscellaneous : - Renamed a bunch of view filter old naming to record filter naming.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR essentially focuses on a refactor of the component hierarchy and naming in advanced filter dropdown, to make it more readable and easy to maintain.
This refactor was required because this area of the code is recursive, so it's better to see the same abstract components in the recursion, instead of trying to guess whether we have the same components than the level above or not.
Also keep in mind that this refactor is meant to separate the advanced filter code path from the view bar simple filter code path, while reusing what's reusable, so here we have a first attempt at finding the sweet spot, that we'll be able to duplicate on other filter dropdown use cases.
Miscellaneous :