Add search pie chart component#81000
Conversation
|
cc: @mateuuszzzzz |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
🚧 @trjExpensify has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
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 |
🦜 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 a5904b5c..1fdc8103 100644
--- a/src/languages/de.ts
+++ b/src/languages/de.ts
@@ -7090,7 +7090,7 @@ Fordere Spesendetails wie Belege und Beschreibungen an, lege Limits und Standard
allMatchingItemsSelected: 'Alle passenden Elemente ausgewählt',
},
topSpenders: 'Top-Ausgaben',
- view: {label: 'Ansehen', table: 'Tabelle', bar: 'Bar'},
+ view: {label: 'Ansehen', table: 'Tabelle', bar: 'Bar', pie: 'Kuchen'},
chartTitles: {
[CONST.SEARCH.GROUP_BY.FROM]: 'Von',
[CONST.SEARCH.GROUP_BY.CARD]: 'Karten',
diff --git a/src/languages/fr.ts b/src/languages/fr.ts
index 55297041..f378bb09 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -7102,7 +7102,7 @@ Exigez des informations de dépense comme les reçus et les descriptions, défin
allMatchingItemsSelected: 'Tous les éléments correspondants sont sélectionnés',
},
topSpenders: 'Plus gros dépensiers',
- view: {label: 'Afficher', table: 'Tableau', bar: 'Barre'},
+ view: {label: 'Afficher', table: 'Tableau', bar: 'Barre', pie: 'Camembert'},
chartTitles: {
[CONST.SEARCH.GROUP_BY.FROM]: 'De',
[CONST.SEARCH.GROUP_BY.CARD]: 'Cartes',
diff --git a/src/languages/it.ts b/src/languages/it.ts
index 169e7064..e4b1f1ec 100644
--- a/src/languages/it.ts
+++ b/src/languages/it.ts
@@ -7079,7 +7079,7 @@ Richiedi dettagli di spesa come ricevute e descrizioni, imposta limiti e valori
allMatchingItemsSelected: 'Tutti gli elementi corrispondenti selezionati',
},
topSpenders: 'Maggiori spenditori',
- view: {label: 'Visualizza', table: 'Tabella', bar: 'Bar'},
+ view: {label: 'Visualizza', table: 'Tabella', bar: 'Bar', pie: 'Torta'},
chartTitles: {
[CONST.SEARCH.GROUP_BY.FROM]: 'Da',
[CONST.SEARCH.GROUP_BY.CARD]: 'Carte',
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index c4669e8b..0e6d0f51 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -7017,7 +7017,7 @@ ${reportName}
allMatchingItemsSelected: '一致する項目をすべて選択済み',
},
topSpenders: 'トップ支出者',
- view: {label: '表示', table: 'テーブル', bar: 'バー'},
+ view: {label: '表示', table: 'テーブル', bar: 'バー', pie: '円グラフ'},
chartTitles: {
[CONST.SEARCH.GROUP_BY.FROM]: '差出人',
[CONST.SEARCH.GROUP_BY.CARD]: 'カード',
diff --git a/src/languages/nl.ts b/src/languages/nl.ts
index e6bd121b..3a9d1156 100644
--- a/src/languages/nl.ts
+++ b/src/languages/nl.ts
@@ -7062,7 +7062,7 @@ Vraag verplichte uitgavedetails zoals bonnetjes en beschrijvingen, stel limieten
allMatchingItemsSelected: 'Alle overeenkomende items geselecteerd',
},
topSpenders: 'Grootste uitgaven',
- view: {label: 'Bekijken', table: 'Tabel', bar: 'Bar'},
+ view: {label: 'Bekijken', table: 'Tabel', bar: 'Bar', pie: 'Cirkeldiagram'},
chartTitles: {
[CONST.SEARCH.GROUP_BY.FROM]: 'Van',
[CONST.SEARCH.GROUP_BY.CARD]: 'Kaarten',
diff --git a/src/languages/pl.ts b/src/languages/pl.ts
index 89532955..7e991feb 100644
--- a/src/languages/pl.ts
+++ b/src/languages/pl.ts
@@ -7049,7 +7049,7 @@ Wymagaj szczegółów wydatków, takich jak paragony i opisy, ustawiaj limity i
allMatchingItemsSelected: 'Wybrano wszystkie pasujące elementy',
},
topSpenders: 'Najwięksi wydający',
- view: {label: 'Zobacz', table: 'Tabela', bar: 'Pasek'},
+ view: {label: 'Zobacz', table: 'Tabela', bar: 'Pasek', pie: 'Wykres kołowy'},
chartTitles: {
[CONST.SEARCH.GROUP_BY.FROM]: 'Od',
[CONST.SEARCH.GROUP_BY.CARD]: 'Karty',
diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts
index 74bc594d..f8fcedc1 100644
--- a/src/languages/pt-BR.ts
+++ b/src/languages/pt-BR.ts
@@ -7051,7 +7051,7 @@ Exija detalhes de despesas como recibos e descrições, defina limites e padrõe
allMatchingItemsSelected: 'Todos os itens correspondentes selecionados',
},
topSpenders: 'Maiores gastadores',
- view: {label: 'Ver', table: 'Tabela', bar: 'Bar'},
+ view: {label: 'Ver', table: 'Tabela', bar: 'Bar', pie: 'Torta'},
chartTitles: {
[CONST.SEARCH.GROUP_BY.FROM]: 'De',
[CONST.SEARCH.GROUP_BY.CARD]: 'Cartões',
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index 41c860b4..9f93b27e 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -6896,7 +6896,7 @@ ${reportName}
allMatchingItemsSelected: '已选择所有匹配的项目',
},
topSpenders: '最高支出者',
- view: {label: '查看', table: '表格', bar: '栏'},
+ view: {label: '查看', table: '表格', bar: '栏', pie: '派'},
chartTitles: {
[CONST.SEARCH.GROUP_BY.FROM]: '来自',
[CONST.SEARCH.GROUP_BY.CARD]: '卡片',
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
jasperhuangg
left a comment
jasperhuangg
left a comment
There was a problem hiding this comment.
few minor comments, overall looking really good, nice job!
| @@ -0,0 +1,46 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Should we investigate the effect of using memoization on this and the other SearchXXXXChart components? Just thinking ahead because we might be embedding these components in chats in the future with VirtualCFO and the ReportScreen is a pretty render-heavy view.
There was a problem hiding this comment.
Yes, this is something we should definitely investigate. @mhawryluk has already started working on embedding charts in Consierge, so we should be able to test this on his branch.
Regarding memoization, all components and hooks are currently compiled with react-compiler except for one hook. We could definitely try to fix this
There was a problem hiding this comment.
ah cool, so probably not as big of an issue as I assumed
MonilBhavsar
left a comment
MonilBhavsar
left a comment
There was a problem hiding this comment.
looks nice, thanks!
|
Perf tests are failing, you might want to merge main |
jasperhuangg
left a comment
jasperhuangg
left a comment
There was a problem hiding this comment.
couple more comments
| // Slices are sorted by absolute value (largest first) for color assignment, | ||
| // so slice indices don't match the original data array. We map back via | ||
| // originalIndex so the tooltip can display the original (possibly negative) value. | ||
| const processedSlices = processDataIntoSlices(data, PIE_CHART_START_ANGLE); |
| // Hover gesture | ||
| const hoverGesture = () => | ||
| Gesture.Hover() | ||
| .onBegin((e) => { | ||
| 'worklet'; | ||
|
|
||
| isHovering.set(true); | ||
| cursorX.set(e.x); | ||
| cursorY.set(e.y); | ||
| tooltipPosition.set({x: e.x, y: e.y - TOOLTIP_BAR_GAP}); | ||
| scheduleOnRN(updateActiveSlice, e.x, e.y); | ||
| }) | ||
| .onUpdate((e) => { | ||
| 'worklet'; | ||
|
|
||
| cursorX.set(e.x); | ||
| cursorY.set(e.y); | ||
| tooltipPosition.set({x: e.x, y: e.y - TOOLTIP_BAR_GAP}); | ||
| scheduleOnRN(updateActiveSlice, e.x, e.y); | ||
| }) | ||
| .onEnd(() => { | ||
| 'worklet'; | ||
|
|
||
| isHovering.set(false); | ||
| scheduleOnRN(setActiveSliceIndex, -1); | ||
| }); | ||
|
|
||
| // Tap gesture for click/tap navigation | ||
| const tapGesture = () => | ||
| Gesture.Tap().onEnd((e) => { | ||
| 'worklet'; | ||
|
|
||
| const {radius, centerX, centerY} = pieGeometry; | ||
| const sliceIndex = findSliceAtPosition(e.x, e.y, centerX, centerY, radius, 0, processedSlices); | ||
|
|
||
| if (sliceIndex >= 0) { | ||
| scheduleOnRN(handleSlicePress, sliceIndex); | ||
| } | ||
| }); |
There was a problem hiding this comment.
already called out by GH review but we should use useCallback
| const pieGeometry = {radius: Math.min(canvasWidth, canvasHeight) / 2, centerX: canvasWidth / 2, centerY: canvasHeight / 2}; | ||
|
|
||
| // Handle hover state updates | ||
| const updateActiveSlice = (x: number, y: number) => { |
There was a problem hiding this comment.
Yeah a lot of stuff can be used with useCallback, can we fix every callback in this file?
| if (data.length === 0) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Just checking if this is expected that we return null here? It seems like when we are loading data, we show a loading spinner. Is a placeholder going to render in this case elsewhere?
There was a problem hiding this comment.
Yeah, this is more of a defensive condition. In practice, Search will render the EmptySearchView component. However, this is something we might want to revisit once we create the PR for concierge chart embeddings, as we will likely use new lightweight components instead of the main heavy search component.
|
I’ve merged main, let’s see if this fixes the performance tests. I had the same issue in another PR today, and merging main resolved it |
| // Slices are sorted by absolute value (largest first) for color assignment, | ||
| // so slice indices don't match the original data array. We map back via | ||
| // originalIndex so the tooltip can display the original (possibly negative) value. | ||
| const processedSlices = processDataIntoSlices(data, PIE_CHART_START_ANGLE); |
|
🚧 @MonilBhavsar has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.3.26-0 🚀
|
|
Hi @borys3kk Is it internal PR? |
|
@izarutskaya No, we should've had QA steps here. You can test this by making sure that the
@borys3kk let's try to make sure to always have QA steps on these PRs going forward. If it's the same as the test steps you can just write "same as tests". |
Ahh, must have missed this when filling the checklist, updated the PR's description |
|
Thanks! |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.26-8 🚀
|
12 participants
Explanation of Change
This PR introduces the SearchPieChart component as part of the Insights project, enabling pie chart visualization in the search view when the user types view:pie with group-by.
New Charts Infrastructure:
Search Integration:
Bar Chart:
Fixed Issues
$ #81028
Tests
Render pie chart with type:expense group-by:category view:pie – verify slices display correctly with distinct colors and appropriate proportions
Hover over slice (web) – verify tooltip shows
<category> • <amount> (<percentage>)Click/tap on slice – verify navigation to filtered search
type:expense category:"<selected>"Verify that no errors appear in the JS console
Offline tests
QA Steps
same as tests
// 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