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

Face global settings, team colors in Dashboard Face, and prefer player Face over blank-face.png #457

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

Face global settings, team colors in Dashboard Face, and prefer player Face over blank-face.png #457

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
14 changes: 12 additions & 2 deletions src/ui/components/NewsBlock.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@ const NewsBlock = ({
});
}

let colors: [string, string, string] | undefined;
if (event.p && event.p.face) {
colors = event.p.face.teamColors as [string, string, string];
}

return (
<div className="card">
<div
Expand All @@ -113,7 +118,8 @@ const NewsBlock = ({
<Badge type={event.type} />
</div>
<div className="d-flex">
{event.p && event.p.imgURL !== "/img/blank-face.png" ? (
{event.p &&
(event.p.imgURL !== "/img/blank-face.png" || event.p.face) ? (
<div
style={{
maxHeight: 90,
Expand All @@ -122,7 +128,11 @@ const NewsBlock = ({
}}
className="flex-shrink-0"
>
<PlayerPicture face={event.p.face} imgURL={event.p.imgURL} />
<PlayerPicture
face={event.p.face}
imgURL={event.p.imgURL}
colors={colors}
/>
</div>
) : null}
<div className="p-2">
Expand Down
9 changes: 7 additions & 2 deletions src/ui/components/PlayerPicture.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const PlayerPicture = ({
}) => {
const [wrapper, setWrapper] = useState<HTMLDivElement | null>(null);
useEffect(() => {
if (face && !imgURL && wrapper) {
if (face && (!imgURL || imgURL == "/img/blank-face.png") && wrapper) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this change (treat the blank-face URL as special, and put it behind a cartoon face in priority) could be confusing. I would prefer keeping the old behavior - imgURL always has priority. You can set imgURL to an empty string (ugh why did I make that the "undefined" value rather than just using undefined) when applying the face objects.

(I think ideally I only would have supported either imgURL or face being present, probably by putting them on the same property of a player object, similar to how they're treated by this PR in the global data. But it's not worth a schema change to fix that now.)

displayFace({
colors,
face,
Expand All @@ -30,14 +30,19 @@ const PlayerPicture = ({
}
}, [face, imgURL, colors, jersey, wrapper]);

if (imgURL) {
// Order of player picture preference: (1) non-blank image > (2) Face JS > (3) blank face
if (imgURL && imgURL !== "/img/blank-face.png") {
return <img alt="Player" src={imgURL} style={imgStyle} />;
}

if (face) {
return <div ref={setWrapper} />;
}

if (imgURL) {
return <img alt="Player" src={imgURL} style={imgStyle} />;
}

return null;
};

Expand Down
13 changes: 11 additions & 2 deletions src/ui/views/GlobalSettings/RealData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,17 @@ const RealData = ({
rows={10}
/>
<div className="text-body-secondary mt-1">
These photos will be used in any new "Real Players" or "Legends"
league you create. Existing leagues will not be affected.
<p>
These photos will be used in any new "Real Players" or "Legends"
league you create. Existing leagues will not be affected.
</p>
<p>
Value can be either URL to player image, or Face object, similar
to what can be generated at{" "}
<a href="https://zengm.com/facesjs/">
https://zengm.com/facesjs/
</a>
</p>
</div>
</div>
</div>
Expand Down
4 changes: 2 additions & 2 deletions src/worker/api/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3517,9 +3517,9 @@ const updateOptions = async (
);
}
for (const [key, value] of Object.entries(realPlayerPhotos)) {
if (typeof value !== "string") {
if (typeof value !== "string" && typeof value !== "object") {
throw new Error(
`Invalid data format in real player photos - value for "${key}" is not a string`,
`Invalid data format in real player photos - value for "${key}" is not a string or Face object`,
);
}
}
Expand Down
12 changes: 10 additions & 2 deletions src/worker/core/league/processPlayerNewLeague.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ const processPlayerNewLeague = async ({
// Do this before augment so it doesn't need to create a face
if (p.srID) {
if (realPlayerPhotos[p.srID] !== undefined) {
p.imgURL = realPlayerPhotos[p.srID];
if (typeof realPlayerPhotos[p.srID] === "string") {
p.imgURL = realPlayerPhotos[p.srID];
} else if (typeof realPlayerPhotos[p.srID] === "object") {
p.face = realPlayerPhotos[p.srID];
}
} else {
const name = p.name ?? `${p.firstName} ${p.lastName}`;

Expand All @@ -36,7 +40,11 @@ const processPlayerNewLeague = async ({
.replace(/ /g, "_")
.toLowerCase()}`;
if (realPlayerPhotos[key] !== undefined) {
p.imgURL = realPlayerPhotos[key];
if (typeof realPlayerPhotos[key] === "string") {
p.imgURL = realPlayerPhotos[p.srID];
} else if (typeof realPlayerPhotos[key] === "object") {
p.face = realPlayerPhotos[p.srID];
}
}
}
}
Expand Down
20 changes: 19 additions & 1 deletion src/worker/views/news.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,28 @@ export const processEvents = async (
{ pid: event.pids[0] },
"noCopyCache",
);
let team;
if (event.tid) {
team = await idb.getCopy.teamsPlus(
{
attrs: ["colors"],
tid: event.tid,
// season: event.season,
},
"noCopyCache",
);
}
Comment on lines +131 to +140
Copy link
Member

Choose a reason for hiding this comment

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

For this change...

  • Would be better to use the getTeamInfoBySeason function, because that can get the jersey colors for a specific season, in the case that they changed over time. Since past seasons can be viewed on this page.
  • Put team colors on the event object, rather than p.face, I think that is clearer. Which could be done in processEvents.
  • In processEvents, it should find all the tids needed to display this page (either from tid parameter, or by scanning all events if tid is undefined), and then get the colors for all of them up front, rather than doing it separately for each event. Since getTeamInfoBySeason has to read from the hard drive for past seasons.
  • Also make a similar change in Headlines.tsx for the dashboard. We should also make sure there's no performance issue from using getTeamInfoBySeason from the dashboard - maybe add a check up front for if it's the current season, and then do await idb.cache.teamSeasons.indexGet("teamSeasonsByTidSeason", [tid, g.get("season")]); if it is, which bypasses hitting the hard drive.

If that sounds like too much, then maybe take the news feed changes out of this PR, and copy the above text to another issue for later :) honestly that might be better to have a separate PR, since it's a little complicated.


if (player) {
if (player.face && team && "colors" in team) {
player.face.teamColors = team.colors as string[];
}
event.p = {
imgURL: player.imgURL,
face: player.imgURL ? undefined : player.face,
face:
player.imgURL && player.imgURL != "/img/blank-face.png"
? undefined
: player.face,
};
numImagesRemaining -= 1;
}
Expand Down