Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Prices Analytics Page #2384

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add Prices Analytics Page #2384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions src/analytics/Prices.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import React, { useMemo } from 'react';
import PropTypes from 'prop-types';

import { Col, Row } from 'reactstrap';

import ErrorBoundary from 'components/ErrorBoundary';
import { compareStrings, SortableTable } from 'components/SortableTable';
import { cardEtchedPrice, cardFoilPrice, cardNormalPrice, cardPrice, cardStatus } from 'utils/Card';

const Prices = ({ cards }) => {
const getValue = (collection, allowedStatus, allowedFinish) => {
Copy link
Contributor

@lunakv lunakv Aug 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const allowedStatuses = [];
if (allowedStatus === 'Owned' || allowedStatus === 'Any') allowedStatuses.push('Ordered', 'Owned', 'Premium Owned');
if (allowedStatus === 'Not Owned' || allowedStatus === 'Any') allowedStatuses.push('Not Owned', 'Proxied');

let priceFn;
switch (allowedFinish) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case 'Non-foil':
priceFn = cardNormalPrice;
break;
case 'Foil':
priceFn = cardFoilPrice;
break;
case 'Etched':
priceFn = cardEtchedPrice;
break;
default:
priceFn = cardPrice;
}

return '$'.concat(
parseFloat(
collection
.map((card) => {
if (allowedStatuses.find((status) => status === cardStatus(card))) return priceFn(card) ?? 0;
Copy link
Contributor

@lunakv lunakv Aug 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return 0;
})
.reduce((acc, price) => acc + price),
).toFixed(2),
);
};

const prices = useMemo(
() =>
['Non-foil', 'Foil', 'Etched', 'All'].map((finish) => ({
label: finish,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

currentValue: getValue(cards, 'Owned', finish),
toComplete: getValue(cards, 'Not Owned', finish),
totalValue: getValue(cards, 'Any', finish),
})),
[cards],
);

return (
<>
<Row>
<Col>
<h4 className="d-lg-block d-none">Prices</h4>
<p>View the expected value of cards owned and unowned.</p>
</Col>
</Row>
<ErrorBoundary>
<SortableTable
columnProps={[
{ key: 'label', title: 'Finish', heading: true, sortable: true },
{ key: 'currentValue', title: 'Current Value', heading: false, sortable: true },
{ key: 'toComplete', title: 'To Complete', heading: false, sortable: true },
{ key: 'totalValue', title: 'Total Value', heading: false, sortable: true },
]}
data={prices}
sortFns={{ label: compareStrings }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

/>
</ErrorBoundary>
</>
);
};

Prices.propTypes = {
cards: PropTypes.arrayOf(PropTypes.shape({})).isRequired,
};

export default Prices;
5 changes: 5 additions & 0 deletions src/pages/CubeAnalysisPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import Playtest from 'analytics/PlaytestData';
import AnalyticTable from 'analytics/AnalyticTable';
import Suggestions from 'analytics/Suggestions';
import Asfans from 'analytics/Asfans';
import Prices from 'analytics/Prices';
import FilterCollapse from 'components/FilterCollapse';
import useQueryParam from 'hooks/useQueryParam';
import useToggle from 'hooks/UseToggle';
Expand Down Expand Up @@ -196,6 +197,10 @@ const CubeAnalysisPage = ({ cube, loginCallback, cubeAnalytics, cards }) => {
name: 'Tokens',
component: (collection, cubeObj) => <Tokens cards={collection} cube={cubeObj} />,
},
{
name: 'Prices',
component: (collection) => <Prices cards={collection} />,
},
];

async function getData(url = '') {
Expand Down