[HOLD #88191] Extract the popup content to make it reusable#88428
[HOLD #88191] Extract the popup content to make it reusable#88428bernhardoj wants to merge 7 commits intoExpensify:mainfrom
Conversation
melvin-bot
Bot
requested review from
aimane-chnaif and
flaviadefaria
and removed request for
a team
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
melvin-bot
Bot
requested review from
a team and
JS00001
and removed request for
a team
|
This PR basically extracts the filter popup content (except amount, date, and report field) so it can be used later in the new filters popup |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import {useLayoutEffect} from 'react'; | ||
|
|
||
| function useFilterCountChange(itemCount: number, callback?: (itemCount: number) => void) { | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
❌ PERF-10 (docs)
This hook uses useLayoutEffect to communicate itemCount to the parent component via the callback prop (which is setItemCount in CommonPopup). This is the pattern described in PERF-10: a child component using an effect to pass derived state to its parent. The hook is used across all filter selector components (CardSelector, CategorySelector, CurrencySelector, InSelector, TagSelector, TaxRateSelector, ExportedToSelector, FeedSelector, WorkspaceSelector, TypeSelector, UserSelector, and the inline components in FilterComponents/index.tsx).
Consider restructuring so that the popup sizing logic is co-located with the filter content rather than requiring an effect-based state sync. For example, each selector could manage its own container sizing, or the CommonPopup wrapper could use a ref-based measurement approach (e.g., onLayout) to determine the content height, removing the need for the child-to-parent effect callback entirely.
Reviewed at: 0eaa94b | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0eaa94bb16
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const [itemCount, setItemCount] = useState(1); | ||
|
|
||
| const applyChanges = () => { | ||
| updateFilterForm({[filterKey]: values} as Partial<SearchAdvancedFiltersForm>); |
There was a problem hiding this comment.
This generic apply/reset path only updates a single key, but the card popup still lets users select card feeds and individual cards together (via CardSelector). For filterKey === cardID, applying now writes only cardID and leaves feed untouched, so deselecting a feed in the card popup has no effect and reset no longer clears feed selections. Previously card apply/reset updated both fields atomically, so this is a behavior regression for existing card+feed filter combinations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@JS00001 do we want to update the card filter here so it only shows individual cards?
There was a problem hiding this comment.
@trjExpensify I'd think the card popover only shows cards, no feeds. Thoughts?
There was a problem hiding this comment.
Yeah, that's my understanding as well.
bernhardoj
changed the title
3 participants
Explanation of Change
Fixed Issues
$ #84877
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari