[Home Page] Create DiscoverSection Component #80807
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 ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
WojtekBoman
force-pushed
the
home-page/discover-section
branch
from
1797040 to
9c7f847
Compare
WojtekBoman
force-pushed
the
home-page/discover-section
branch
from
9c7f847 to
16f423c
Compare
WojtekBoman
changed the title
|
@codex review |
melvin-bot
bot
requested review from
ZhenjaHorbach and
trjExpensify
and removed request for
a team
|
@ZhenjaHorbach 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] |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| return ( | ||
| <WidgetContainer title={translate('homePage.announcements')}> | ||
| <View style={{height: 400}} /> |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The hardcoded height value 400 is a magic number without documentation or named constant.
Suggested fix:
Define a named constant for the placeholder height:
const ANNOUNCEMENTS_PLACEHOLDER_HEIGHT = 400;
function AnnouncementsSection() {
const {translate} = useLocalize();
return (
<WidgetContainer title={translate('homePage.announcements')}>
<View style={{height: ANNOUNCEMENTS_PLACEHOLDER_HEIGHT}} />
</WidgetContainer>
);
}Or add a comment explaining the placeholder:
{/* Placeholder height for announcements section - will be replaced in upcoming PR */}
<View style={{height: 400}} />Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
src/pages/home/DiscoverSection.tsx
Outdated
| onPress={handlePress} | ||
| style={styles.mb8} | ||
| wrapperStyle={styles.pl8} | ||
| numberOfLinesTitle={2} |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The hardcoded value 2 for numberOfLinesTitle is a magic number without documentation.
Suggested fix:
Define a named constant or add a comment explaining why exactly 2 lines:
const MENU_ITEM_TITLE_MAX_LINES = 2;
<MenuItemWithTopDescription
shouldShowRightIcon
title={isCurrentUserPolicyAdmin ? translate('homePage.discoverSection.menuItemTitleAdmin') : translate('homePage.discoverSection.menuItemTitleNonAdmin')}
description={translate('homePage.discoverSection.menuItemDescription')}
onPress={handlePress}
style={styles.mb8}
wrapperStyle={styles.pl8}
numberOfLinesTitle={MENU_ITEM_TITLE_MAX_LINES}
/>Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED, {canBeMissing: true}); | ||
| const styles = useThemeStyles(); | ||
|
|
||
| const handlePress = () => { |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The Linking.openURL call lacks error handling. If the URL is malformed or the system cannot open the link, the error will be silent.
Suggested fix:
Add error handling and user feedback:
const handlePress = () => {
const url = getTestDriveURL(shouldUseNarrowLayout, introSelected, isCurrentUserPolicyAdmin);
Linking.openURL(url).catch((error) => {
Log.error('Failed to open test drive URL', error);
// Optionally show user feedback
});
};Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Technically correct but inconsistent with existing patterns. The codebase doesn't do this anywhere else.
src/pages/home/ForYouSection.tsx
Outdated
| const {translate} = useLocalize(); | ||
| const [accountID] = useOnyx(ONYXKEYS.SESSION, {canBeMissing: false, selector: accountIDSelector}); | ||
|
|
||
| const handleGoToSearch = () => { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The four navigation handler functions (handleGoToSearch, handleGoToApproveSearch, handleGoToPaySearch, handleGoToExportSearch) contain duplicated logic with only minor variations in parameters.
Reasoning: This code duplication increases maintenance overhead. If the navigation pattern or query building logic changes, all four functions need to be updated.
Suggested fix:
Consolidate into a reusable helper function:
const navigateToSearchWithQuery = (queryParams: Record<string, unknown>) => {
Navigation.navigate(
ROUTES.SEARCH_ROOT.getRoute({
query: buildQueryStringFromFilterFormValues(queryParams),
}),
);
};
const handleGoToSearch = () => {
navigateToSearchWithQuery({
type: CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT,
action: CONST.SEARCH.ACTION_FILTERS.SUBMIT,
from: [`${accountID}`],
});
};
const handleGoToApproveSearch = () => {
navigateToSearchWithQuery({
type: CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT,
action: CONST.SEARCH.ACTION_FILTERS.APPROVE,
to: [`${accountID}`],
});
};
// Similar pattern for other handlers...Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
I think it's okay for now
| const styles = useThemeStyles(); | ||
| const {translate} = useLocalize(); | ||
| const [isSelfTourViewed = false] = useOnyx(ONYXKEYS.NVP_ONBOARDING, {selector: hasSeenTourSelector, canBeMissing: true}); | ||
|
|
There was a problem hiding this comment.
❌ PERF-10 (docs)
The useEffect calling confirmReadyToOpenApp() is used for side effects rather than synchronizing with an external system. This pattern can lead to timing issues and unnecessary complexity.
Reasoning: useEffect should be reserved for synchronizing with external systems (DOM, network, third-party libraries), not for triggering actions on mount. If this needs to run on mount, consider if it belongs in the component at all, or if it should be handled by the parent/router.
Suggested fix:
If this is truly needed on component mount, consider:
- Moving this logic to the app initialization layer (where routing happens)
- Or if it must stay here, add a comment explaining why useEffect is necessary:
// confirmReadyToOpenApp must be called after HomePage mounts to signal
// that the app is ready to handle deep links and navigation
useEffect(() => {
confirmReadyToOpenApp();
}, []);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
src/components/WidgetContainer.tsx
Outdated
| </View> | ||
| )} | ||
| <View style={[styles.flexShrink1, styles.flexGrow1, styles.flexRow, styles.alignItemsCenter, styles.gap2]}> | ||
| {!!title && ( |
There was a problem hiding this comment.
❌ CONSISTENCY-4 (docs)
The titleColor prop is defined with a default value via the null coalescing operator (titleColor ?? theme.text), but this creates redundancy. If titleColor is optional, it should either:
- Have a default value in the function signature, or
- Not use the
??operator if it's meant to be required
Reasoning: The current pattern makes the component API unclear. Is titleColor optional or required? The implementation suggests it's optional, but then the fallback should be in the prop signature.
Suggested fix:
type WidgetContainerProps = {
/** Custom color for the title text. Defaults to theme.text if not provided */
titleColor?: string;
// ... other props
};
function WidgetContainer({
children,
icon,
title,
titleColor, // Remove the default here
iconWidth = variables.iconSizeNormal,
iconHeight = variables.iconSizeNormal,
}: WidgetContainerProps) {
const styles = useThemeStyles();
const theme = useTheme();
// Use titleColor or fall back to theme.text
const actualTitleColor = titleColor ?? theme.text;
return (
<View style={styles.widgetContainer}>
{/* ... */}
{!!title && (
<Text style={styles.getWidgetContainerTitleStyle(actualTitleColor)}>{title}</Text>
)}
</View>
);
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
It's a good point, but we don't have access to the theme in the function declaration because it's obtained with the hook
src/languages/en.ts
Outdated
| announcements: 'Announcements', | ||
| discoverSection: { | ||
| title: 'Discover', | ||
| menuItemTitleNonAdmin: 'Learn how to create expenses and submit reports', |
There was a problem hiding this comment.
We need to add a period here
|
And we have lint issues |
This comment has been minimized.
This comment has been minimized.
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/de.ts b/src/languages/de.ts
index cb882589..ab9aca8e 100644
--- a/src/languages/de.ts
+++ b/src/languages/de.ts
@@ -8211,6 +8211,16 @@ Hier ist ein *Testbeleg*, um dir zu zeigen, wie es funktioniert:`,
},
fabGpsTripExplained: 'Zur GPS-Ansicht wechseln (Schnellaktion)',
},
+ homePage: {
+ forYou: 'Für dich',
+ announcements: 'Ankündigungen',
+ discoverSection: {
+ title: 'Entdecken',
+ menuItemTitleNonAdmin: 'Erfahren Sie, wie Sie Ausgaben erstellen und Berichte einreichen.',
+ menuItemTitleAdmin: 'Erfahren Sie, wie Sie Mitglieder einladen, Genehmigungsworkflows bearbeiten und Firmenkarten abstimmen.',
+ menuItemDescription: 'Sehen Sie, was Expensify in 2 Minuten kann',
+ },
+ },
};
// IMPORTANT: This line is manually replaced in generate translation files by scripts/generateTranslations.ts,
// so if you change it here, please update it there as well.
diff --git a/src/languages/fr.ts b/src/languages/fr.ts
index 37cbe700..52e3d950 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -8217,6 +8217,16 @@ Voici un *reçu test* pour vous montrer comment cela fonctionne :`,
},
fabGpsTripExplained: 'Aller à l’écran GPS (action flottante)',
},
+ homePage: {
+ forYou: 'Pour vous',
+ announcements: 'Annonces',
+ discoverSection: {
+ title: 'Découvrir',
+ menuItemTitleNonAdmin: 'Découvrez comment créer des dépenses et soumettre des rapports.',
+ menuItemTitleAdmin: 'Apprenez à inviter des membres, à modifier les circuits d’approbation et à rapprocher les cartes de l’entreprise.',
+ menuItemDescription: 'Découvrez ce qu’Expensify peut faire en 2 minutes',
+ },
+ },
};
// IMPORTANT: This line is manually replaced in generate translation files by scripts/generateTranslations.ts,
// so if you change it here, please update it there as well.
diff --git a/src/languages/it.ts b/src/languages/it.ts
index ed81344a..1b17f3cd 100644
--- a/src/languages/it.ts
+++ b/src/languages/it.ts
@@ -6987,8 +6987,8 @@ Richiedi dettagli di spesa come ricevute e descrizioni, imposta limiti e valori
[CONST.SEARCH.GROUP_BY.CARD]: 'Carta',
[CONST.SEARCH.GROUP_BY.WITHDRAWAL_ID]: 'ID prelievo', //_/\__/_/ \_,_/\__/\__/\_,_/
[CONST.SEARCH.GROUP_BY.CATEGORY]: 'Categoria',
- [CONST.SEARCH.GROUP_BY.MERCHANT]: 'Commerciante',
- [CONST.SEARCH.GROUP_BY.TAG]: 'Etichetta',
+ [CONST.SEARCH.GROUP_BY.MERCHANT]: 'Esercente',
+ [CONST.SEARCH.GROUP_BY.TAG]: 'Tag',
[CONST.SEARCH.GROUP_BY.MONTH]: 'Mese',
},
feed: 'Feed',
@@ -8197,6 +8197,16 @@ Ecco una *ricevuta di prova* per mostrarti come funziona:`,
},
fabGpsTripExplained: 'Vai alla schermata GPS (azione flottante)',
},
+ homePage: {
+ forYou: 'Per te',
+ announcements: 'Annunci',
+ discoverSection: {
+ title: 'Scopri',
+ menuItemTitleNonAdmin: 'Scopri come creare spese e inviare report.',
+ menuItemTitleAdmin: 'Scopri come invitare membri, modificare i flussi di approvazione e riconciliare le carte aziendali.',
+ menuItemDescription: 'Scopri cosa può fare Expensify in 2 minuti',
+ },
+ },
};
// IMPORTANT: This line is manually replaced in generate translation files by scripts/generateTranslations.ts,
// so if you change it here, please update it there as well.
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index 53da5dd9..6e70da58 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -6923,10 +6923,10 @@ ${reportName}
reimbursable: '精算対象',
purchaseCurrency: '購入通貨',
groupBy: {
- [CONST.SEARCH.GROUP_BY.FROM]: '送信者',
+ [CONST.SEARCH.GROUP_BY.FROM]: '差出人',
[CONST.SEARCH.GROUP_BY.CARD]: 'カード',
[CONST.SEARCH.GROUP_BY.WITHDRAWAL_ID]: '出金ID',
- [CONST.SEARCH.GROUP_BY.CATEGORY]: 'カテゴリー',
+ [CONST.SEARCH.GROUP_BY.CATEGORY]: 'カテゴリ',
[CONST.SEARCH.GROUP_BY.MERCHANT]: '加盟店',
[CONST.SEARCH.GROUP_BY.TAG]: 'タグ',
[CONST.SEARCH.GROUP_BY.MONTH]: '月',
@@ -8110,6 +8110,16 @@ Expensify の使い方をお見せするための*テストレシート*がこ
},
fabGpsTripExplained: 'GPS画面へ移動（フローティングアクション）',
},
+ homePage: {
+ forYou: 'あなた向け',
+ announcements: 'お知らせ',
+ discoverSection: {
+ title: '発見',
+ menuItemTitleNonAdmin: '経費の作成方法とレポートの提出方法を学びましょう。',
+ menuItemTitleAdmin: 'メンバーの招待方法、承認ワークフローの編集方法、会社カードの照合方法について学びましょう。',
+ menuItemDescription: '2 分で Expensify でできることを確認する',
+ },
+ },
};
// IMPORTANT: This line is manually replaced in generate translation files by scripts/generateTranslations.ts,
// so if you change it here, please update it there as well.
diff --git a/src/languages/nl.ts b/src/languages/nl.ts
index 6841f8db..2ddc9f82 100644
--- a/src/languages/nl.ts
+++ b/src/languages/nl.ts
@@ -6970,8 +6970,8 @@ Vraag verplichte uitgavedetails zoals bonnetjes en beschrijvingen, stel limieten
[CONST.SEARCH.GROUP_BY.CARD]: 'Kaart',
[CONST.SEARCH.GROUP_BY.WITHDRAWAL_ID]: 'Opname-ID',
[CONST.SEARCH.GROUP_BY.CATEGORY]: 'Categorie',
- [CONST.SEARCH.GROUP_BY.MERCHANT]: 'Verkoper',
- [CONST.SEARCH.GROUP_BY.TAG]: 'Label',
+ [CONST.SEARCH.GROUP_BY.MERCHANT]: 'Handelaar',
+ [CONST.SEARCH.GROUP_BY.TAG]: 'Tag',
[CONST.SEARCH.GROUP_BY.MONTH]: 'Maand',
},
feed: 'Feed',
@@ -8173,6 +8173,16 @@ Hier is een *testbon* om je te laten zien hoe het werkt:`,
},
fabGpsTripExplained: 'Ga naar GPS-scherm (Zwevende actie)',
},
+ homePage: {
+ forYou: 'Voor jou',
+ announcements: 'Aankondigingen',
+ discoverSection: {
+ title: 'Ontdek',
+ menuItemTitleNonAdmin: 'Leer hoe je uitgaven aanmaakt en rapporten indient.',
+ menuItemTitleAdmin: 'Leer hoe u leden uitnodigt, goedkeuringsworkflows bewerkt en bedrijfskaarten afstemt.',
+ menuItemDescription: 'Ontdek wat Expensify in 2 minuten kan doen',
+ },
+ },
};
// IMPORTANT: This line is manually replaced in generate translation files by scripts/generateTranslations.ts,
// so if you change it here, please update it there as well.
diff --git a/src/languages/pl.ts b/src/languages/pl.ts
index 4f135c5c..d5865831 100644
--- a/src/languages/pl.ts
+++ b/src/languages/pl.ts
@@ -8159,6 +8159,16 @@ Oto *paragon testowy*, który pokazuje, jak to działa:`,
},
fabGpsTripExplained: 'Przejdź do ekranu GPS (przycisk akcji)',
},
+ homePage: {
+ forYou: 'Dla ciebie',
+ announcements: 'Ogłoszenia',
+ discoverSection: {
+ title: 'Odkryj',
+ menuItemTitleNonAdmin: 'Dowiedz się, jak tworzyć wydatki i składać raporty.',
+ menuItemTitleAdmin: 'Dowiedz się, jak zapraszać członków, edytować przepływy akceptacji i uzgadniać karty firmowe.',
+ menuItemDescription: 'Zobacz, co Expensify potrafi w 2 minuty',
+ },
+ },
};
// IMPORTANT: This line is manually replaced in generate translation files by scripts/generateTranslations.ts,
// so if you change it here, please update it there as well.
diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts
index f1d837ef..0f08e19e 100644
--- a/src/languages/pt-BR.ts
+++ b/src/languages/pt-BR.ts
@@ -6960,7 +6960,7 @@ Exija detalhes de despesas como recibos e descrições, defina limites e padrõe
[CONST.SEARCH.GROUP_BY.CARD]: 'Cartão',
[CONST.SEARCH.GROUP_BY.WITHDRAWAL_ID]: 'ID da retirada',
[CONST.SEARCH.GROUP_BY.CATEGORY]: 'Categoria',
- [CONST.SEARCH.GROUP_BY.MERCHANT]: 'Comerciante',
+ [CONST.SEARCH.GROUP_BY.MERCHANT]: 'Estabelecimento',
[CONST.SEARCH.GROUP_BY.TAG]: 'Etiqueta',
[CONST.SEARCH.GROUP_BY.MONTH]: 'Mês',
},
@@ -8168,6 +8168,16 @@ Aqui está um *recibo de teste* para mostrar como funciona:`,
},
fabGpsTripExplained: 'Ir para a tela de GPS (Ação flutuante)',
},
+ homePage: {
+ forYou: 'Para você',
+ announcements: 'Anúncios',
+ discoverSection: {
+ title: 'Descobrir',
+ menuItemTitleNonAdmin: 'Aprenda a criar despesas e enviar relatórios.',
+ menuItemTitleAdmin: 'Aprenda a convidar membros, editar fluxos de aprovação e reconciliar cartões corporativos.',
+ menuItemDescription: 'Veja o que o Expensify pode fazer em 2 minutos',
+ },
+ },
};
// IMPORTANT: This line is manually replaced in generate translation files by scripts/generateTranslations.ts,
// so if you change it here, please update it there as well.
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index 0203fdfc..a78ef128 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -6804,10 +6804,10 @@ ${reportName}
purchaseCurrency: '购买货币',
groupBy: {
[CONST.SEARCH.GROUP_BY.FROM]: '来自',
- [CONST.SEARCH.GROUP_BY.CARD]: '卡片',
- [CONST.SEARCH.GROUP_BY.WITHDRAWAL_ID]: '提现 ID',
+ [CONST.SEARCH.GROUP_BY.CARD]: '卡',
+ [CONST.SEARCH.GROUP_BY.WITHDRAWAL_ID]: '提现编号',
[CONST.SEARCH.GROUP_BY.CATEGORY]: '类别',
- [CONST.SEARCH.GROUP_BY.MERCHANT]: '商家',
+ [CONST.SEARCH.GROUP_BY.MERCHANT]: '商户',
[CONST.SEARCH.GROUP_BY.TAG]: '标签',
[CONST.SEARCH.GROUP_BY.MONTH]: '月',
},
@@ -7933,6 +7933,16 @@ ${reportName}
locationServicesRequiredModal: {title: '需要访问位置信息', confirm: '打开设置', prompt: '请在设备设置中允许位置访问，以开始 GPS 距离跟踪。'},
fabGpsTripExplained: '前往 GPS 屏幕（悬浮操作）',
},
+ homePage: {
+ forYou: '为你',
+ announcements: '公告',
+ discoverSection: {
+ title: '发现',
+ menuItemTitleNonAdmin: '了解如何创建费用并提交报表。',
+ menuItemTitleAdmin: '了解如何邀请成员、编辑审批流程以及对公司卡进行对账。',
+ menuItemDescription: '看看 Expensify 在 2 分钟内能为你做什么',
+ },
+ },
};
// IMPORTANT: This line is manually replaced in generate translation files by scripts/generateTranslations.ts,
// so if you change it here, please update it there as well.
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
The card keeps flickering out in the test build. Also we wanna reduce the bottom padding of the card by like 12px (I'll find the real value once I can use web inspector) |
|
ios looks good! 
|
|
Prettier failing |
| discoverSection: { | ||
| title: 'Descubrir', | ||
| menuItemTitleNonAdmin: 'Aprende a crear gastos y enviar informes.', | ||
| menuItemTitleAdmin: 'Aprende a invitar a miembros, editar flujos de aprobación y conciliar tarjetas corporativas.', | ||
| menuItemDescription: 'Descubre lo que Expensify puede hacer en 2 minutos', | ||
| }, |
|
Just a note that I cleared the mobile padding issues in person so that'll be a follow-up |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
And I think we want the second line here to be bold, too: We should update the UI screenshot here to include the new sidebar navigation too 🤣 |
|
@shawnborton @dubielzyk-expensify @WojtekBoman I made a follow up issue here |
|
Wonderful, thanks! |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.11-16 🚀
|
|
@WojtekBoman @mountiny @ZhenjaHorbach Could you please add a precondition how to get a Discover Section with a new Expensify account? |
9 participants
Explanation of Change
This PR adds the Discover section to
HomePage.tsx. It's visible only whenselfTourViewedisfalse.Wide layout:
Narrow layout:
Fixed Issues
$ #79980
PROPOSAL:
Tests
Precondition: access to
newDotHomebetaOffline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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