Skip to content

BL-10968 Add location tables to stats#427

Merged
hatton merged 2 commits into
masterfrom
BL-10968LocationStats
Feb 22, 2022
Merged

BL-10968 Add location tables to stats#427
hatton merged 2 commits into
masterfrom
BL-10968LocationStats

Conversation

@hatton

@hatton hatton commented Feb 21, 2022

Copy link
Copy Markdown
Member

Also some refactoring to remove duplicated code and code around sorting (comparing) number strings which doesn't seem to be needed because we already convert all number-strings to numbers before giving them to the grid.


This change is Reviewable

@andrew-polk andrew-polk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @hatton)


package.json, line 37 at r1 (raw file):

        "@nivo/bar": "^0.62.0",
        "@sentry/browser": "^6.0.0",
        "@types/lodash": "^4.14.178",

In the end it probably doesn't matter, but we've kept the @types dependencies in devDependencies to date.


src/components/statistics/CollectionStatsPage.tsx, line 99 at r1 (raw file):

            {
                label: l10n.formatMessage({
                    id: "stats.locations",

Should this have a new entry in CodeStrings?

Code quote:

"stats.locations"

src/components/statistics/CollectionStatsPage.tsx, line 118 at r1 (raw file):

    }, [l10n]);

    const [currentScreenIndex, setCurrentScreenIndex] = useState(4); // TODO do not commit

// TODO do not commit

You were just testing me, right?

Code quote:

// TODO do not commit

src/components/statistics/GridWrapper.tsx, line 8 at r1 (raw file):

import React from "react";

export const StatsGridWrapper: React.FunctionComponent<{

Mismatch between the file name and component name


src/components/statistics/StatsLocationScreen.tsx, line 39 at r1 (raw file):

        { name: "region", title: "Region", l10nId: "region" },
        { name: "city", title: "City", l10nId: "city" },
        { name: "reads", title: "Reads", l10nId: "reads" },

I guess we are not internationalizing at this time?

@hatton hatton left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: 4 of 10 files reviewed, 5 unresolved discussions (waiting on @andrew-polk)


package.json, line 37 at r1 (raw file):

Previously, andrew-polk wrote…

In the end it probably doesn't matter, but we've kept the @types dependencies in devDependencies to date.

Done.


src/components/statistics/CollectionStatsPage.tsx, line 99 at r1 (raw file):

Previously, andrew-polk wrote…

Should this have a new entry in CodeStrings?

Right, no localization yet in stats


src/components/statistics/CollectionStatsPage.tsx, line 118 at r1 (raw file):

Previously, andrew-polk wrote…

// TODO do not commit

You were just testing me, right?

Done.


src/components/statistics/StatsLocationScreen.tsx, line 39 at r1 (raw file):

Previously, andrew-polk wrote…

I guess we are not internationalizing at this time?

Right, no localization yet in stats


src/components/statistics/GridWrapper.tsx, line 8 at r1 (raw file):

Previously, andrew-polk wrote…

Mismatch between the file name and component name

Done.

@andrew-polk andrew-polk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hatton)


src/components/statistics/CollectionStatsPage.tsx, line 118 at r2 (raw file):

    }, [l10n]);

    const [currentScreenIndex, setCurrentScreenIndex] = useState(1);

Doesn't this mean that the Overview will not be the initial one loaded? Is that intentional?

Code quote:

useState(1)

@hatton hatton force-pushed the BL-10968LocationStats branch from d7996cc to 833655f Compare February 22, 2022 21:33

@hatton hatton left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @andrew-polk)


src/components/statistics/CollectionStatsPage.tsx, line 118 at r2 (raw file):

Previously, andrew-polk wrote…

Doesn't this mean that the Overview will not be the initial one loaded? Is that intentional?

Done.

@hatton hatton merged commit 8d09a76 into master Feb 22, 2022
@andrew-polk andrew-polk deleted the BL-10968LocationStats branch March 7, 2022 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants