Dashboard Pages v2#6262
Conversation
RobertJoonas
force-pushed
the
v2-dashboard-pages
branch
from
136b190 to
05e3235
Compare
RobertJoonas
force-pushed
the
v2-dashboard-pages
branch
from
05e3235 to
fc2c551
Compare
|
- instead of importing Metric from types/query-api.d.ts, define a Metric type in metrics.ts (including all internally available metrics) and use that as the single Metric type everywhere - add metric labelling logic in metrics.ts - move MetricValue type from fetch-main-graph.ts -> api.ts, and use it for Top Stats metric value as well
This commit introduces use-order-by-legacy.ts and use-order-by-legacy.test.ts files as copies of the current modules. The intent is to make the diff easier to review -- all components currently using the hook will use the legacy variant instead, and the actual file will change into v2 in the following commit.
* breakdown-modal.tsx -> breakdown-modal-legacy.tsx * breakdown-table.tsx -> breakdown-table-legacy.tsx * table.tsx -> table-legacy.tsx ...and use the legacy variants everywhere. The existing files will be transformed into v2 in the following commit.
- Cache whole api responses (including meta and query), not only results - AfterFetchData & afterFetchNextPage -> onDataReady. The function now also runs when fetch is a cache hit. - Avoid usePaginatedQueryAPI for IndexBreakdown. Unnecessary complexity because pagination is not needed. Instead let IndexBreakdown call useQuery directly
- add order by [dim, asc] to Index and DetailsBreakdowns - Define pagination field in StatsQuery and ReportParams
| (orderBy.length | ||
| ? Object.fromEntries(orderBy) | ||
| : Object.fromEntries(defaultOrderBy), | ||
| : Object.fromEntries(defaultOrderBy)) as Record<Metric, SortDirection>, |
There was a problem hiding this comment.
nitpick, non-blocking: Casting ( as ...) should ideally be avoided. If we declare the const to be of this type (const orderByDictionary: Record<Metric, SortDirection> = ...), we get Typescript compile errors when the const isn't set to this value, which most of the times is useful. Casting is a bit of an escape hatch, to be used for situations when the compiler can't possibly know. Why didn't we need to cast before and now do? Is it because Order = [string, SortDirection] should be [Metric, SortDirection]?
There was a problem hiding this comment.
Casting ( as ...) should ideally be avoided. If we declare the const to be of this type (const orderByDictionary: Record<Metric, SortDirection> = ...), we get Typescript compile errors when the const isn't set to this value, which most of the times is useful. Casting is a bit of an escape hatch, to be used for situations when the compiler can't possibly know.
Makes sense! Thanks for explaining!
Why didn't we need to cast before and now do? Is it because Order = [string, SortDirection] should be [Metric, SortDirection]?
Good question. Yeah I believe it was related to the Metric type but to be honest I don't remember why I added it at some point.
However, I already changed the Order type to be [string, SortDirection] rather than [Metric, SortDirection] (that's because we need to order by dimensions themselves too and there's no explicit Dimension type).
I guess since useOrderBy is only dealing with metric ordering, the correct approach would be to have two separate types:
OrderBy = [string, SortDirection][]instats-query.tsMetricOrderBy = [Metric, SortDirection][]inuse-order-by.ts
Happy to tackle that as a follow-up if you think that makes sense. For now I've simply removed the as ... cast. 2f64b3a
There was a problem hiding this comment.
I see, I didn't even think of dimension sort. Your idea to split the type is good (and we can also import Dimensions from query-api.d.ts in the FE and do [Metric | Dimensions, SortDirection][]).
| validateOrderBy( | ||
| parsed, | ||
| metrics.filter((m) => m.sortable) | ||
| metrics.filter((m) => isSortable(m)) |
There was a problem hiding this comment.
| metrics.filter((m) => isSortable(m)) | |
| metrics.filter(isSortable) |
This is a bit shorter
There was a problem hiding this comment.
I just noticed there's one more instance of the same thing in the same file that can be shortened
apata
left a comment
apata
left a comment
There was a problem hiding this comment.
Great work! I believe it's very close to being mergable, with the only blocker for my approval being the page urls now wrapping to multiple lines in details views. The other things we discussed in sync, like using the queryKey to build the statsQuery or maintaining pages test suite for longer aren't blockers from my POV.
Using up exactly all the space enables horizontal scroll on Safari which we don't want on destkop.
2 participants
Changes (code-structure oriented description)
useQueryFadeIncomponent with a simple set of Tailwind classes. The animation duration is set to 300ms to make it noticeable.usePaginatedQueryAPIhook which uses a StatsQuery object in itsqueryKeyrather than a DashboardState.afterFetchDataprops ->onDataReady(Same as for IndexBreakdown. Instead of a separate function for next page vs initial page, the single handler now receives all pages of results which will be needed in the conversions report to determine whether revenue metrics should be displayed or not)onDataReadycallback instead ofafterFetchDataandafterFetchNextPage-- need to fire the callback when response is read from cache too, not only after a real fetch call)apiResponse.queryandapiResponse.metaby letting TanStack query cache both as part of the response.ColumnConfigurationfor defining columns. In order to generate columns, actual data is needed. Unlike the legacyColumnConfiguration, the new version enforces bothrenderLabelandrenderCellas functions, therefore leaving the rendering components (BreakdownTable and IndexBreakdownRenderer) dumb about any special cases around dimension/metric values/labels.metrics.ts. Note: it is already extended to components that are not yet migrated (e.g. conversions). Currently dead code but will be used soon.Recap of user facing changes
Tests
Changelog
Documentation
Dark mode