Custom tables 2 #406

Vac1911 · 2022-01-24T19:52:19Z

Look I made it so you can change what columns tables have:

and you can reorder them with drag and drop easily

Since the previous pull request:

I've improved the backwards compatibility so a ton of tables don't have to be updated. Currently I've only upgraded freeAgents, tradingBlock, trade, playerStats, playerRatings, and playerBios.
Made the TableConfig Class unserializable so that it's methods can be used in the ui.
Improved the API for adding dynamic columns (such as the include column on the trading block) and updating columns through new methods in TableConfig
All other changes talked about in the previous pull request.

dumbmatter

This is just a quick initial review with mostly minor comments. I still need to think about the big picture more. This definitely is a lot better than the previous PR, and easier to review too. Very impressive you have gotten it this far!

For more minor comments, run yarn run lint :)

src/ui/util/columns/templates/Name.tsx

src/ui/util/columns/templates/Negotiate.tsx

src/ui/util/columns/templates/Weight.tsx

src/ui/views/TradingBlock.tsx

src/worker/db/connectMeta.ts

src/ui/components/DataTable/CustomizeColumns.tsx

src/ui/util/TableConfig.ts

dumbmatter · 2022-01-27T18:18:21Z

src/ui/util/TableConfig.ts

@@ -0,0 +1,93 @@
+import { idb } from "../../worker/db";


A file in the ui folder should not be importing anything from worker. Either make two separate versions of TableConfig, one for ui and one for worker, or make one base class in common and extend that for use in the worker.

I'm not sure it makes sense to share the same class for ui and worker... seems every function is only called in one of those places.

dumbmatter · 2022-01-27T18:18:46Z

src/ui/util/TableConfig.ts

+	}
+
+	static unserialize(_config: TableConfig) {
+		const serialized = cloneDeep(_config);


Use helpers.deepCopy instead.

doing this broke things while building but will try again after doing this:
#406 (comment)

dumbmatter · 2022-01-27T18:19:40Z

src/ui/util/TableConfig.ts

+	public fallback: string[];
+	public columns: MetaCol[];
+	public tableName: string;
+	public vars: LeagueVars | object;


There are tons of lint errors (including this) if you run yarn run lint

only a couple hundred

I fixed some but many were due to the type Player bindings which we discussed on discord, not done yet but made some progress.

dumbmatter · 2022-01-27T18:21:07Z

src/worker/api/index.ts

 		});
+		const config: TableConfig = new TableConfig("tradingBlock", [


const config: TableConfig = can just be const config =` - in general, only manually specify types when necessary.

dumbmatter · 2022-01-27T18:26:29Z

src/ui/util/columns/templates/Name.tsx

+import { PlayerNameLabels } from "../../../components";
+
+export default ({ p, c, vars }: TemplateProps) => (
+	<PlayerNameLabels


What if additional props need to be passed to PlayerNameLabels? What if they only need to be conditionally passed? For example

zengm/src/ui/views/PlayerStats.tsx

Line 139 in 8a59d09

season={season === "career" ? undefined : p.stats.season}

This is a broader point, it affects a lot of these template components.

In the MetaCol type there is a property called options and there could be an option for season.
However because it is likely more than 1 column needs to know what season is selected, I've added it to TableConfig.vars property

It all gets very messy and complicated... because in some places, "career" is a valid season, but in other places it's not. Ideally there would be some TypeScript-enforced way to say "the rows of data passed to the table need these properties" but even that is hard to think of what exactly it means, given what I mentioned above about some variables meaning different things in different tables. This is going to be so hard to get right without bugs/regressions. Especially since playersPlus returns any currently.

It's possible we'll need some way to pass accessor functions to template functions, like a way to say "by default skills is in p.ratings.skills, or supply a callback function that takes p and returns skills".

dumbmatter · 2022-01-27T18:27:35Z

src/ui/util/columns/getTemplate.ts

+import type { Player } from "../../../common/types";
+import type { TableConfig } from "../TableConfig";
+
+export default function (p: Player, c: MetaCol, config: TableConfig) {


What if it's a table showing things besides players? Like teams or something else?

Well currently it wouldn't be customizable but eventually there could be a set of columns for teams too. I'm open to ideas thou.

Is the team data structured similarly enough to the player data where we could have an interface which describes both?

I think there's no good way to do it.

Ideally the templates would be generic functions, where the input extends whatever is required. Like

export default function({row, c, config}: { row: { tid: number, region: string, name: string, } } & TemplateProps) {

Or maybe better:

export default function({row, c, config}: TemplateProps<{ tid: number, region: string, name: string, }>) {

Then if we specify what the allowed template functions are for a certain table, I think it would be possible to use TypeScript to take a union of all the row types and confirm that the row objects in the table actually conform to the required format. The problem is, the row objects are dynamic based on the selected columns. I think that makes it too difficult to use TypeScript for this. So we'll lose the type safety we currently have (at least for tables displaying data not typed as any - many player tables are any, but not all, and most non-player tables are not). Which means, like I said in my other comment, this is going to be very hard to do without messing something up.

While I'm rambling here, basically my biggest concerns are:

This stuff is kind of error prone (what I wrote above in this comment)

Maybe I need to rewrite perGame/per36/total handling to allow them to be individually selected, before doing column customiation

What about tables that dynamically show/hide a column based on some conditions (like Player Stats showing All Seasons, or League Stats where past seasons don't have some info) - would need some way to specify that a column, even when selected, may not be possible to display.

dumbmatter · 2022-01-27T18:30:11Z

src/ui/views/PlayerStats.tsx

 		return {
-			key,
-			data: [


We lost all the special stuff here, right? Use of nameAbbrev, conditional setting of season, custom sort/search values, ageAtDeath rather than just age, conditional season column...

Related to this... there are several tables that only display a column sometimes, based on some input condition. How should that be handled when columns are customized?

These drop downs should still work? what is the nameAbbrev and custom search ?

nameAbbbrev displays only the initials of the first name, because this is a wide table. Arguably there's no need for it. But other stuff in various places there is need for, like setting the season passed to PlayerNameLabels which is used to set the season of the player stats/ratings popup.

Plus the other stuff I mentioned. I'm talking about the functionality of the table, not the dropdowns at the top. Like look what happens when a dead player appears when it's showing career totals. Or when you switch to "all seasons".

dumbmatter · 2022-01-27T18:35:37Z

src/ui/components/DataTable/Header.tsx

+			);
+		}
+
+		return (


Moving the sort arrows to the foreground means columns get wider. Not sure if that is a good thing. Probably not, since some tables are already quite wide on moblie.

Could this go back to how it was before? Click sorts, drag reorders? The roster page has table handles that are separately clickable and draggable, so I think it's possible.

Vac1911 and others added 5 commits January 21, 2022 16:42

improved implementation of customizing columns

b89c3c9

TradingBlock.tsx fix

8f0f101

Improved Table Customization

f390acc

Merge branch 'master' into custom-tables

bd210bd

highlightCol and footers

5367fa9

dumbmatter requested changes Jan 25, 2022

View reviewed changes

Vac1911 added 7 commits January 25, 2022 08:55

backwards compatible supercols

10e0d3c

Frontend Changes to CustomizeColumns.tsx

9d4cb5d

CustomizeColumns.tsx increased performance of getting initialColumns

dc16fb3

bug fixes

1c10603

created types LeagueVars and TemplateProps

97547ad

Customize Columns visual changes

d609c78

Only show progs in preseason

7d86379

Vac1911 requested a review from dumbmatter January 27, 2022 16:50

dumbmatter reviewed Jan 27, 2022

View reviewed changes

Vac1911 added 4 commits January 27, 2022 15:39

set League Var interface

c3088da

Fixing Linting Issues

199dded

Bugfix

dbf7b75

getPlayerTable

0ce3e13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom tables 2 #406

Custom tables 2 #406

Vac1911 commented Jan 24, 2022

dumbmatter left a comment

dumbmatter Jan 27, 2022

dumbmatter Jan 27, 2022

dumbmatter Jan 27, 2022

Vac1911 Jan 28, 2022

dumbmatter Jan 27, 2022

Vac1911 Jan 27, 2022

Vac1911 Jan 28, 2022

dumbmatter Jan 27, 2022

dumbmatter Jan 27, 2022 •

edited

Vac1911 Jan 27, 2022

dumbmatter Jan 27, 2022

dumbmatter Jan 27, 2022

Vac1911 Jan 27, 2022 •

edited

Vac1911 Jan 27, 2022

dumbmatter Jan 27, 2022

dumbmatter Jan 27, 2022

dumbmatter Jan 27, 2022

Vac1911 Jan 27, 2022

dumbmatter Jan 27, 2022

dumbmatter Jan 27, 2022

		});
		const config: TableConfig = new TableConfig("tradingBlock", [

Custom tables 2 #406

Are you sure you want to change the base?

Custom tables 2 #406

Conversation

Vac1911 commented Jan 24, 2022

dumbmatter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dumbmatter Jan 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vac1911 Jan 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dumbmatter Jan 27, 2022 •

edited

Vac1911 Jan 27, 2022 •

edited