feat(in-app-analytics): New rule hit table + Filters bar + data fetching - #1150
Conversation
7f7855b to
b8b9b5f
Compare
b8b9b5f to
9a7184a
Compare
9a7184a to
0446975
Compare
siiick
changed the title
Pascal-Delange
left a comment
Pascal-Delange
left a comment
There was a problem hiding this comment.
As far as I was able to judge the react code, this looks ok to me. I left some minor comments, that may be addressed in a further PR. Let's have a look at the functional QA once it's in staging.
@ChibiBlasphem can have a look at the PR tomorrow perhaps.
| "decisions.tooltip.monthly": "mois de {{date}}", | ||
| "decisions.tooltip.weekly": "semaine du {{date}} <Br/>(Semaine {{weekNumber}})", | ||
| "filters.add_compare_period": "Ajouter une période de comparaison" | ||
| "filters.add_compare_period": "Ajouter une période de comparaison", |
There was a problem hiding this comment.
We'll do a full review of translations, but at the end of the project I think (so this cycle)
|
|
||
| const handleExportCsv = () => { | ||
| if (!currentDataGroup) return; | ||
| const rows = percentage ? currentDataGroup.data.ratio : currentDataGroup.data.absolute; |
There was a problem hiding this comment.
To make it simpler, I'd rather we just return the csv with total values, in every case.
| className="text-s w-full truncate px-4 font-medium text-purple-65" | ||
| colSpan={table.getHeaderGroups()[0]?.headers.length ?? 5} | ||
| > | ||
| See more + |
| }; | ||
| } | ||
| } catch { | ||
| // ignore malformed q |
There was a problem hiding this comment.
for convenience, add a console log ? (this is client side or server side ?)
There was a problem hiding this comment.
I will change it in another PR (work todo on this part)
| decline: '#E99B8E', | ||
| }; | ||
|
|
||
| export const analyticsQuery = z.object({ |
There was a problem hiding this comment.
For the graph "1" especially, the backend will now also accept a timezone string, to take into account to group decisions by time bucket.
There was a problem hiding this comment.
(let's do that in a follow-up PR however)
| if (currentParams.scenarioId !== nextParams.scenarioId) return true; | ||
| const keys = ['q'] as const; | ||
| return keys.some((k) => currentUrl.searchParams.get(k) !== nextUrl.searchParams.get(k)); | ||
| export function shouldRevalidate() { |
There was a problem hiding this comment.
I assume this is a remix thing ?
There was a problem hiding this comment.
Yes it is. Since we load all dynamic data through queries and invalidate them programmatically when we update the URL query parameters, we want Remix to avoid revalidating the route data.
| { | ||
| type: 'select', | ||
| name: 'scenario', | ||
| placeholder: 'Select scenario', |
There was a problem hiding this comment.
even if it's annoying, let's have some translations here
2 participants
Uh oh!
There was an error while loading. Please reload this page.