-
Notifications
You must be signed in to change notification settings - Fork 132
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.
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
Custom tables 2 #406
base: master
Are you sure you want to change the base?
Custom tables 2 #406
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
:)
@@ -0,0 +1,93 @@ | |||
import { idb } from "../../worker/db"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
|
||
static unserialize(_config: TableConfig) { | ||
const serialized = cloneDeep(_config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use helpers.deepCopy instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing this broke things while building but will try again after doing this:
#406 (comment)
src/ui/util/TableConfig.ts
Outdated
public fallback: string[]; | ||
public columns: MetaCol[]; | ||
public tableName: string; | ||
public vars: LeagueVars | object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are tons of lint errors (including this) if you run yarn run lint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only a couple hundred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed some but many were due to the type Player bindings which we discussed on discord, not done yet but made some progress.
}); | ||
const config: TableConfig = new TableConfig("tradingBlock", [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const config: TableConfig =
can just be const config =` - in general, only manually specify types when necessary.
import { PlayerNameLabels } from "../../../components"; | ||
|
||
export default ({ p, c, vars }: TemplateProps) => ( | ||
<PlayerNameLabels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
import type { Player } from "../../../common/types"; | ||
import type { TableConfig } from "../TableConfig"; | ||
|
||
export default function (p: Player, c: MetaCol, config: TableConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if it's a table showing things besides players? Like teams or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the team data structured similarly enough to the player data where we could have an interface which describes both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return { | ||
key, | ||
data: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
); | ||
} | ||
|
||
return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:
freeAgents
,tradingBlock
,trade
,playerStats
,playerRatings
, andplayerBios
.TableConfig
Class unserializable so that it's methods can be used in the ui.TableConfig