Migrate Conversions, Props reports to /query endpoint#6440
Conversation
|
apata
force-pushed
the
query/migrate-conversions-props
branch
from
2b1a42f to
b6ea299
Compare
| const BAR_COLOR = 'bg-red-50 group-hover/row:bg-red-100' | ||
|
|
||
| type ConversionsProps = { | ||
| afterFetchData?: (data: QueryApiResponse) => void |
There was a problem hiding this comment.
Nit: Let's rename this to onDataReady to be consistent?
| // isPageViewGoal currently has no return statement and resolves to | ||
| // `undefined` at runtime; cast preserves the historical truthiness check. | ||
| const isPageview = isPageViewGoal(goalName) as unknown as boolean |
There was a problem hiding this comment.
Since isPageViewGoal never returns anything, we can safely get rid of this const I guess. The !undefined below currently evaluates to true anyway so it's redundant in the && chain.
FTR: I believe the intent was to not switch tab to Mode.PROPS if the goal filter being applied is a pageview goal. Looks like that broke at some point then. Maybe accidentally, but I guess it makes sense to change tab even in this case. The props tab will definitely be more interesting than the single goal item :)
| function renderImportedQueryUnsupportedWarning(): ReactNode { | ||
| if (mode === Mode.CONVERSIONS) { | ||
| return ( | ||
| <ImportedQueryUnsupportedWarning |
There was a problem hiding this comment.
Suggestion (non-blocking): What do you think about renaming this component to FunnelApiImportedWarningBubble? Otherwise we'd have ImportedQueryUnsupportedWarning and ImportedWarningBubble, making it hard to understand how they're different.
When I first introduced the new ImportedWarningBubble component, I was hoping we could use that here too. Now that I've went down a familiar rabbit hole trying to get rid of ImportedDataInView, I've realised that there's no obvious solution on the table for the "Funnels" and "Explore" tabs.
For custom props and conversions however, we could use ImportedWarningBubble. For conversions, it'd only be a matter of passing the whole API response. For props, we'd have to also extend the component with a message? option.
Then the FunnelApiImportedWarningBubble would only depend on importedDataInView and nothing else.
Might be worth adding a module-level comment there too:
Renders an imported warning bubble for the "Funnels" and "Explore" tabs. Currently, while the funnel and exploration queries silently ignore everything related to imports, we still want to let the user know that imports are not included in those reports. Therefore, we rely on Top Stats response to know whether imported data is in range or not, and if it is, we render the warning bubble.
All that said though, probably best to do this separately in a follow-up
| import { NonTimeDimension } from '../../stats-query' | ||
| import { FilterInfo } from '../../components/drilldown-link' | ||
|
|
||
| const BAR_COLOR = 'bg-red-50 group-hover/row:bg-red-100' |
There was a problem hiding this comment.
Could we export this from here and use in props.tsx and special-goal-prop-breakdown.tsx too?
| ) | ||
| } | ||
|
|
||
| function getGoalsFilterInfo( |
There was a problem hiding this comment.
this appears to be an exact duplicate of the same function in stats/modals/conversions.tsx. Let's share a single function.
| [dashboardState, dimensions] | ||
| ) | ||
|
|
||
| const columnsHiddenForAllNull = useMemo((): Set<Metric> => { |
There was a problem hiding this comment.
Very similar to the one in index-breakdown.tsx, with the only exception being the shape of the data. Maybe there's a way to deduplicate it? A hook like useHiddenMetricColumns?
| {...props} | ||
| barClassName={BAR_COLOR} | ||
| text={value} | ||
| getFilterInfo={() => ({ |
There was a problem hiding this comment.
Could we use makeGetFilterInfo from stats/modals/props.tsx here to deduplicate?
|
|
||
| return ( | ||
| <div className="w-full" style={{ minHeight: `${MIN_HEIGHT}px` }}> | ||
| <IndexBreakdown |
There was a problem hiding this comment.
Something we already talked about but leaving a note JIC: Should pass a bigger column width for desktop. Same in special goal prop breakdown.
| ['visitors', 'desc'], | ||
| ...dimensions.map((dim): OrderByEntry => [dim, 'asc']) | ||
| ...dimensions | ||
| .filter((dim) => dim !== 'event:goal') |
There was a problem hiding this comment.
We're making sure here that we’re not trying to order by event:goal which is unsupported. This logic is currently duplicated in both index and details breakdowns. Would be nice to define that limitation in a single place, e.g.: reports-config.ts? Either by setting an explicit dimensionOrderBy field for the goals report, or just exporting a const CANNOT_ORDER_BY_DIMENSIONS
| return { | ||
| dimensions: [`event:props:${propKey}` as NonTimeDimension], | ||
| metricsByContext: { | ||
| realtimeMetrics: ['visitors', 'events'], |
There was a problem hiding this comment.
Already talked about it but highlighting again JIC: missing conversion rate in realtime.
2 participants
Changes
Migrates Conversions and Props reports to /query endpoint
Tests
Changelog
Documentation
Dark mode