Skip to content
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

Fix bug and refactored advanced filter field selection dropdown #11103

Merged
merged 4 commits into from
Mar 24, 2025

Conversation

lucasbordeau
Copy link
Contributor

This PR is a first step towards isolating each filter dropdown use case, here we isolate advanced filter field selection dropdown from view bar filter field selection dropdown.

Isolation of advanced filter field selection logic

We reimplement the previously generic logic into AdvancedFilterFieldSelectMenu and AdvancedFilterSubFieldSelectMenu components.

For now it contains duplicated code but, the end goal is to factorize what's common to all object filter dropdowns in small hooks where possible, at the end of the code path isolation first step, which will be done for applyFilter and selectFilter logic that will be able to be deleted after code path isolation.

A new component ObjectFilterDropdownFilterSelectMenuItemV2 has been created to expose an onClick method instead of computing logic that tries to guess where it is located, which allows to verticalize what happens when we select a field in each specific use case, one layer above.

Created the hook useSelectFieldUsedInAdvancedFilterDropdown which contains the logic for field selection for advanced filter field select dropdown specific use case, the first example of a good verticalized and unique place to handle field selection in the context of advanced filter.

The naming useAdvancedFilterDropdown was lying and is now useAdvancedFilterFieldSelectDropdown which is now self-explanatory.

useAdvancedFilterFieldSelectDropdown has been removed from the main object filter dropdown, thus isolating advanced filters field select dropdown from the generic object filter field select dropdown.

Miscellaneous

Removed states that were used in the generic filter dropdown to guess whether it was in advanced filter context or not.

… logic

- Renamed useAdvancedFilterDropdown to useAdvancedFilterFieldSelectDropdown and removed its usage in generic object filter dropdown
- Created AdvancedFilterFieldSelectMenu and AdvancedFilterSubFieldSelectMenu to handle field selection in advanced filter field select dropdown
- Created useSelectFieldUsedInAdvancedFilterDropdown to handle field selection in advanced filter field select dropdown
- Removed advancedFilterViewFilterGroupIdComponentState
- Removed advancedFilterViewFilterIdComponentState
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR refactors the advanced filter dropdown logic by isolating it from the generic filter dropdown, improving code organization and maintainability through dedicated components and hooks.

  • Added AdvancedFilterFieldSelectMenu and AdvancedFilterSubFieldSelectMenu components to handle specific advanced filter field selection logic
  • Introduced ObjectFilterDropdownFilterSelectMenuItemV2 with explicit onClick prop for better control flow and separation of concerns
  • Created useSelectFieldUsedInAdvancedFilterDropdown hook to verticalize advanced filter field selection logic
  • Removed shared states like advancedFilterViewFilterIdComponentState and advancedFilterViewFilterGroupIdComponentState in favor of explicit implementations
  • Renamed useAdvancedFilterDropdown to useAdvancedFilterFieldSelectDropdown for clarity and specificity

15 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile

@charlesBochet charlesBochet merged commit e83e7b3 into main Mar 24, 2025
50 checks passed
@charlesBochet charlesBochet deleted the refactor/advanced-filter-dropdown-field-selection branch March 24, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants