Add Prices Analytics Page #2384
Conversation
There was a problem hiding this comment.
If you're still willing to work on this (I see it's been a while since you submitted the PR), I left some comments on the changes. I'll also add that specifically for changes to the UI, it's a very pleasant nice-to-have to include some screenshots into the PR description showing what the changes look like.
| import { cardEtchedPrice, cardFoilPrice, cardNormalPrice, cardPrice, cardStatus } from 'utils/Card'; | ||
|
|
||
| const Prices = ({ cards }) => { | ||
| const getValue = (collection, allowedStatus, allowedFinish) => { |
There was a problem hiding this comment.
Does this need to be nested inside the React component? It doesn't depend on any props, so I don't see a reason to recreate it on every render.
| parseFloat( | ||
| collection | ||
| .map((card) => { | ||
| if (allowedStatuses.find((status) => status === cardStatus(card))) return priceFn(card) ?? 0; |
There was a problem hiding this comment.
The condition can be simplified as a call to allowedStatuses.includes, and possibly removed in favor of a .filter call prior to the map, depending on personal preference.
The map call also isn't strictly necessary, you can get the price in the reducer, but I suppose that's also more of a question of taste.
| const prices = useMemo( | ||
| () => | ||
| ['Non-foil', 'Foil', 'Etched', 'All'].map((finish) => ({ | ||
| label: finish, |
There was a problem hiding this comment.
I feel like this labeling is a little unclear, in that it's not really self-evident what the cell values mean. For example, for Foil -> To Complete, does that mean the value of "Not Owned" cards that are marked as foil, the value of "Owned" cards that are not marked as foil but could be turned into foils, or the total value of all cards, regardless of status, that that aren't marked as foil? I can see justifications for any of the three.
Feel free to come up with your own way of clarifying this, but for my money, since the headings really just represent ownership status, why not label them that way? Meaning you'd just use "Owned" as a heading instead of "Current Value" etc. I feel like that's much more clear, if ever so slightly less natural.
| if (allowedStatus === 'Not Owned' || allowedStatus === 'Any') allowedStatuses.push('Not Owned', 'Proxied'); | ||
|
|
||
| let priceFn; | ||
| switch (allowedFinish) { |
There was a problem hiding this comment.
This is OK, just preemptively flagging this as something that we might want to extract into a separate function in the future, should this functionality (getting the price of a card in a specified finish) become more widely used.
| { key: 'totalValue', title: 'Total Value', heading: false, sortable: true }, | ||
| ]} | ||
| data={prices} | ||
| sortFns={{ label: compareStrings }} |
There was a problem hiding this comment.
I think you might have a problem if you try to sort by any of the price columns. You're passing the price data as strings (prefixed with "$"), but the default comparison function is just (a, b) => a - b, which will produce NaN if passed any two strings, and thus completely fail as a comparator.
I would suggest keeping the data returned from getValue as simple floats so they can be properly sorted by the table, and pass an appropriate renderFn into the column props to format the prices. (As an additional bonus, this achieves better separation of concerns, as getValue will become responsible only for calculating the price, not also for formatting it).
#850
Took a shot in the dark for columns and labels. Also just getting into the swing of things so if there's any standards I'm not adhering to I'd love to make sure that's corrected early.