Repository navigation
104405 trips table - #104562
104405 trips table#104562suneox wants to merge 4 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
| * 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
This branch has not been deployed
2 participants
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
Trips table
Sorted by status
View opens the trip room
Tapping the row opens the trip room
Narrow layout
Beta off: still the expense search