Repository navigation
104405 trips table - #104562
104405 trips table#104562suneox wants to merge 8 commits into
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
allgandalf
left a comment
allgandalf
left a comment
There was a problem hiding this comment.
please merge main, there are conflicts. ESLint is also red on SearchUIUtilsTest.ts (reportID shadow and an unsafe cast), and checklist fails on the unticked boxes
on Trips, Display still has Group by (picking one just resets the sort) and Filters still has Amount, Merchant etc, which end in Something went wrong. ok to leave for #104406 ?
| CONST.SEARCH.TABLE_COLUMNS.IN, | ||
| CONST.SEARCH.TABLE_COLUMNS.AVATAR, | ||
| CONST.SEARCH.TABLE_COLUMNS.VIOLATIONS, | ||
| // The trip search doesn't sort by these yet |
There was a problem hiding this comment.
backend sorts trips by date, title and from too, can we map Start date, Trip name and Traveler instead of disabling them ?
| * A trip row in search results. Tapping the row or its View action opens the trip room. | ||
| */ | ||
| function TripListItem<TItem extends ListItem>({item, columns = [], isFocused, showTooltip, isDisabled, onSelectRow, onFocus, shouldSyncFocus, isLastItem}: TripListItemProps<TItem>) { | ||
| const tripItem = item as unknown as TripListItemType; |
There was a problem hiding this comment.
ESLint fails on this cast, can we use isTripListItemType here ?
| function getTripSections(data: OnyxTypes.SearchResults['data'], formatPhoneNumber: LocaleContextProps['formatPhoneNumber'], translate: LocalizedTranslate): [TripListItemType[], number] { | ||
| const trips = Object.keys(data) | ||
| .filter(isReportEntry) | ||
| .map((key) => data[key] as SearchTrip) |
There was a problem hiding this comment.
this and the cast in getSortedTripData push the file over its seatbelt limit, that's the ESLint failure
| }, []); | ||
| const {isBetaEnabled} = usePermissions(); | ||
| const isTravelApprovalEnabled = isBetaEnabled(CONST.BETAS.TRAVEL_APPROVAL); | ||
| const handleSearchAction = useCallback( |
There was a problem hiding this comment.
this file compiles with React Compiler, can we drop the useCallback while we're here ? same in SearchPageNarrow CLEAN-REACT-PATTERNS-0
| ); | ||
|
|
||
| const columnConfig = useMemo(() => getSearchColumns(type, icons, groupBy, isExpenseReportView), [type, groupBy, icons, isExpenseReportView]); | ||
| const columnConfig = useMemo( |
There was a problem hiding this comment.
same here, file compiles so no need for this useMemo CLEAN-REACT-PATTERNS-0
| shouldShowSorting | ||
| groupBy={validGroupBy} | ||
| isExpenseReportView={isExpenseReportType} | ||
| isExpenseReportView={isExpenseReportType || shouldReturnTripRooms} |
There was a problem hiding this comment.
why do trips pass isExpenseReportView ? a short comment would help CONSISTENCY-18
There was a problem hiding this comment.
Trip rows use the same layout as the expense report view: a fixed-width Total column followed by the Action column, with no flex on Total. I've added short comment to explain
| * Do not use directly, use only via `getSections()` facade. | ||
| */ | ||
| /** | ||
| * @private |
There was a problem hiding this comment.
this went in below getTaskSections' JSDoc, so getTaskSections has none now. can we move it up ? also CONSISTENCY-10 bans @private
| /** Columns to show on wide screens */ | ||
| columns: SearchColumnType[]; | ||
|
|
||
| /** Whether to show tooltips */ |
There was a problem hiding this comment.
NAB: these just repeat the prop names, same for status in TripStatusCell. fine to drop them CONSISTENCY-10
|
@allgandalf All review comments have been resolved. Since this PR is built on top of #104536, I'll update pr status once #104536 is merged
Yes, we should handle this in #104406
based on the scope of that issue. It should also cover the use case of manually typing |
|
@thelullabyy 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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@thelullabyy The auto-assignment is incorrect. @allgandalf will be review the travel approval feature |
|
@codex review |
Reviewer Checklist
Screenshots/Videos
|
![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: 055bb2b8a8
ℹ️ 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".
|
cc @Krishna2323 - it seems like the table above is not using auto-sizing cells? |
… editing trip columns
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
core design doc change
Explanation of Change
Adds the Trips table in Spend (Travel → Trips), one row per trip room, as described in the design doc (Detailed Implementation → Release 1, step 3). Everything is behind the
travelApprovalbeta.Fixed Issues
$ #104405
PROPOSAL:
Tests
Setup: sign in as a user who has Expensify Travel trips, in a workspace set up for Expensify Travel.
travelApprovalis an explicit-only beta, so turn it on in Troubleshoot → Beta overrides even if the account hasall.travelApprovalbeta off in Beta overrides and opentype:tripfrom the search bar. Verify it shows the expense search as before, with no Trips table.Offline tests
QA Steps
travelApprovalbeta for an account with several Expensify Travel trips, in a workspace set up for Expensify Travel.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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.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
CleanShot.2026-10-23.at.12.54.05.1.mp4