Award editor #331

luso97 · 2020-11-25T19:40:12Z

Feature #302 finished. All the problems you mentioned are fixed so I think it´s all done. i finally created an external component for the selects so it would be pretty easy to change it if neccesary, and all the error handling is done outside of this component.

# Conflicts: # package.json # src/worker/api/index.ts

dumbmatter

This is awesome, thank you so much!

I left a bunch of comments. You can handle as many of them as you want, just let me know. I could definitely take it from here and address them all if you don't want to deal with it. Or if you do want to deal with it, that's great too. Or in between :)

Additional comments that don't fit inline with the code:

Run yarn run lint and there are some warnings that should be addressed.
The react-select inputs look bad in dark mode (Tools > Global Settings)
Add link from season summary page to form (only in God Mode)
If there is no player winning an award (such as MIP in the first season), if you add a player for that award, you can't remove it again. i guess ideally it should be possible for "nobody" to be selected for any award
In BBGM, editing the All-Defensive team doesn't seem to work, no changes get saved. All-League does work.
Would be nice to combine EditAwardsBasketball and EditAwardsFootball into one file, where all the UI code is shared and there is just some configuration object that differs across sports. Similar to how it works in Settings.tsx, where some settings are only available in FBGM or BBGM.

dumbmatter · 2020-11-26T04:18:26Z

src/ui/views/EditAwards.basketball.tsx

+						const teamSelect = element["players"].map(
+							(player: any, j: number) => {
+								return (
+									<div className="col form-group">


Add key={j} - without that, you see a warning message in the JS console from React. Same on line 286.

dumbmatter · 2020-11-26T04:23:36Z

src/worker/core/season/awards.ts

+		const p = await idb.cache.players.get(pid);
+		if (p) {
+			p.awards = p.awards.filter(
+				x => x.season != season || x.type! in awardsToDelete,


x.type! in awardsToDelete - how does this even work? If awardsToDelete is an array, this should always return false, because in looks at the keys of an object (which for an array are the numeric indices) and x.type is not a number.

This is so it doesn´t delete League champs. Basically what it does is just leaves those awards that are not changed on the form and those awards from other season

I understand that's what you intend for it to do, but x.type! in awardsToDelete will always be false. Try and see. Probably should be !awardsToDelete.includes(x.type)

Doesn´t work since awardsToDelete is an object so includes is not a method. I have seen that The x.type! in awardsToDelete is not always false. For example if the player has a finalChamps award or an all star award, those awards is not included in awardsToDelete so the filter applies. I have tried erasing that part of the filter and it deletes the finals champ awards, which is what I´m trying to avoid. And I have printed in the console what the x.type!in awardsToDelete filter returns and it returns all the all star and champ awards, so that when updating the awards those are not changed whatsoever

awardsToDelete: string[], a few lines up says it's an array, and the only time this function is called, it's given a value of Object.values(awardNames) which is an array.

dumbmatter · 2020-11-26T04:27:27Z

src/worker/views/editAwards.tsx

+	let awards = await idb.getCopy.awards({
+		season,
+	});
+


Would be nice to default to last season if we know there are no awards for the current season, like:

if (!awards) { if (g.get("season") === season && g.get("phase") <= PHASE.PLAYOFFS) { season -= 1; awards = await idb.getCopy.awards({ season, }); } }

(This is done after checking for the presence of awards in case some weird situation happens where there actually is an awards object before this season's playoffs end)

dumbmatter · 2020-11-26T04:28:52Z

src/ui/util/menuItems.tsx

+				godMode: true,
+				league: true,
+				path: ["edit_awards"],
+				text: "Edit awards",


For consistency, name it "Edit Awards" and move after "Delete Old Data"

dumbmatter · 2020-11-26T04:29:43Z

src/ui/components/SelectMultiple.tsx

+import React, { createRef } from "react";
+import Select from "react-select";
+
+class SelectReact extends React.Component<


This really doesn't matter, and you don't have to do anything about it, but I would have used hooks here. Generally I like the hooks API better, and I do have some shared hooks I use in multiple components that don't work with class components. Not relevant here, but maybe it would be in the future.

dumbmatter · 2020-11-26T04:35:20Z

src/ui/views/EditAwards.football.tsx

+							options={players}
+							player={awards["finalsMvp"]}
+							award="finalsMvp"
+							changing={handleChange.bind(this)}


Don't need .bind(this) in any of these, because handleChange does not use this so it doesn't matter what it is. Also this is undefined here anyway, since this is in an arrow function. Another nice thing about not using classes, you can ignore all the nonsense about what this is!

dumbmatter · 2020-11-26T04:40:26Z

src/worker/api/index.ts

+		process.env.SPORT === "basketball"
+			? (["mvp", "roy", "smoy", "dpoy", "mip", "finalsMvp"] as const)
+			: (["mvp", "dpoy", "droy", "oroy", "finalsMvp"] as const);
+	const awardNames =


Since these are also defined exactly the same in doAwards.basketball.ts and doAwards.football.ts, they should be shared. Maybe as a new object in constants.basketball.ts and constants.football.ts which can then be imported both here and in the doAwards files.

Same with simpleAwards I think.

Actually, a bunch of the code below seems to be duplicated from the doAwards files, so maybe all of this should just be put into a single function which gets called here and in the doAwards files, maybe called makeAwardsByPlayer(awards). I think that would be everything here before the awardsByPlayerToDelete stuff.

dumbmatter · 2020-11-26T04:44:51Z

src/worker/core/season/awards.ts

@@ -321,11 +327,29 @@ const saveAwardsByPlayer = async (
 		}
 	}
 };
+const deleteAwardsByPlayer = async (
+	awardsByPlayer: AwardsByPlayer,
+	conditions: Conditions,


This is never used, so it should not be an argument to this function.

dumbmatter · 2020-11-26T04:45:36Z

src/worker/core/season/awards.ts

 		if (p) {
 			for (const awardByPlayer of awardsByPlayer) {
 				if (awardByPlayer.pid === pid) {
 					p.awards.push({
-						season: g.get("season"),
+						season: season ? season : g.get("season"),


Fun new syntax! This can be season: season ?? g.get("season"). Or better yet, it can just be season, if you change line 265 to:

season: number = g.get("season"),

dumbmatter · 2020-11-26T04:49:44Z

src/worker/core/season/awards.ts

+			const p2 = await idb.getCopy.players({
+				pid: pid,
+			});
+			p = p2 as PlayerFiltered;


PlayerFiltered is any, so might as well just call this any if that's what needs to be done. I feel like it should work to just make line 311.

this should work:

p = await idb.getCopy.players({ pid: pid, });

...except my return value from getCopy.players is wrong, should be undefined rather than void. If changing that causes other problems, don't worry about it, I can deal with this :)

Convert to one file and some requested changes. UI can get better in dark mode and some testing to do

Just missing state in UI files and merge EditBasketball and EditFootball

luso97 · 2020-12-08T18:24:29Z

Everything works now and I have changed everything you mentioned. The only thing I didn´t change is the x.type! in awardsToDelete . for the reason I posted above. I also haven´t merged both EditFootball and EditBasketball (last bullet point in the comment above), if you think is neccesary I can look at that issue but could take some time, since I have looked at Setttings.tsx and haven´t figured out how to equate it to the code I have done already. I don´t know if you want it exactly like it is there or it was just an example.

Edit: Actually, if you don´t want it like in Settings.tsx it would be pretty easy to do I think, just some process.env.sport==football or basketball in the individual awards, and maybe 2 handleChange since the method differs a little between sports. So if that suits you, I´ll do it that way.

luso97 · 2020-12-09T20:01:05Z

I have finally merged both files in one with what I said in the last comment, since I saw it was pretty easy. So, I would say the whole functionality is done barring any error or bad practice.

dumbmatter · 2020-12-11T01:29:48Z

I have finally merged both files in one with what I said in the last comment, since I saw it was pretty easy. So, I would say the whole functionality is done barring any error or bad practice.

Thanks. I think I'm going to try making a few minor tweaks and then merge it...

dumbmatter · 2020-12-11T03:36:14Z

src/ui/views/EditAwards.tsx

+							name: p.name,
+							tid: p.tid,
+							abbrev: p.abbrev,
+							pts: p.currentStats == undefined ? 0.0 : p.currentStats.pts,


(don't fix this, I'm editing stuff now)

0.0 vs 0 in Python is meaningful, but JS has only floats, so 0 and 0.0 are exactly the same thing.

dumbmatter · 2020-12-11T03:47:24Z

src/worker/core/season/awards.ts

+	}
+};
+const makeAwardsByPlayer = async (awards: any, conditions: Conditions) => {
+	const awardsByPlayer: AwardsByPlayer = [];


Before this function was created, awardsByPlayer was not an empty array at this point, it had the league leaders in it. So league leader awards are missing from players here :)

dumbmatter · 2020-12-11T05:07:14Z

I did some work on this branch https://github.com/dumbmatter/gm-games/tree/luso97-award-editor and deployed it to https://beta.basketball-gm.com/ and https://beta.football-gm.com/ for a little user testing. Please let me know if you notice something I broke!

Luis Morán added 8 commits October 11, 2020 11:05

Added links and start of page

669e7a0

Issue almost complete, barring react select replacement

096571b

Added football award edition

32105cc

Added error fixing when player already in teams

041c217

Final commit barring testing

3ff4fc3

Feature implemented

8e683c8

Merge branch 'forkBranch/master' into award-editor

935e755

# Conflicts: # package.json # src/worker/api/index.ts

Minor error and code cleaning

e0a656a

dumbmatter requested changes Nov 26, 2020

View reviewed changes

Luis Morán added 6 commits December 3, 2020 11:04

Warnings, nobody as award getter and dark mode

d822c9a

Changed constnats and fixed errors in pull request

2a531c7

Convert to one file and some requested changes. UI can get better in dark mode and some testing to do

Style and some unnecessary code erased

9418769

Just missing state in UI files and merge EditBasketball and EditFootball

Fixed states so they are only one object

f82fb54

Merge branch 'forkBranch/master' into award-editor

bf856a3

Links added and some bugs fixed

8533fcc

Merged EditBasketball and EditFootball in one file

19ee430

dumbmatter reviewed Dec 11, 2020

View reviewed changes

dumbmatter approved these changes Dec 11, 2020

View reviewed changes

dumbmatter merged commit 19ee430 into zengm-games:master Dec 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Award editor #331

Award editor #331

luso97 commented Nov 25, 2020 •

edited

dumbmatter left a comment

dumbmatter Nov 26, 2020

dumbmatter Nov 26, 2020

luso97 Dec 3, 2020

dumbmatter Dec 6, 2020

luso97 Dec 8, 2020 •

edited

dumbmatter Dec 11, 2020

dumbmatter Nov 26, 2020

dumbmatter Nov 26, 2020

dumbmatter Nov 26, 2020

dumbmatter Nov 26, 2020

dumbmatter Nov 26, 2020

dumbmatter Nov 26, 2020

dumbmatter Nov 26, 2020

dumbmatter Nov 26, 2020

dumbmatter Nov 26, 2020

luso97 commented Dec 8, 2020 •

edited

luso97 commented Dec 9, 2020

dumbmatter commented Dec 11, 2020

dumbmatter Dec 11, 2020

dumbmatter Dec 11, 2020

dumbmatter commented Dec 11, 2020

Award editor #331

Award editor #331

Conversation

luso97 commented Nov 25, 2020 • edited

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

luso97 Dec 8, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luso97 commented Dec 8, 2020 • edited

luso97 commented Dec 9, 2020

dumbmatter commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dumbmatter commented Dec 11, 2020

luso97 commented Nov 25, 2020 •

edited

luso97 Dec 8, 2020 •

edited

luso97 commented Dec 8, 2020 •

edited