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.

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

Exploding files: Part 2 (AdminPage) #179

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

theredwillow
Copy link
Contributor

The AdminPage was a considerable 1072 lines long, making this PR lofty enough with just one file explosion. I have gotten it down to just 146 lines now.

There is some considerable prop drilling for the time being. These series of PR's will help illuminate what contexts we need for issue #168 in a future PR.

@theredwillow theredwillow marked this pull request as ready for review January 24, 2025 04:11
@joshzcold
Copy link
Owner

!test

@theredwillow
Copy link
Contributor Author

@joshzcold Could you clarify what !test means? I'd like to make sure I address it properly.

@joshzcold
Copy link
Owner

joshzcold commented Jan 24, 2025

@theredwillow oh sorry. That kicks off e2e tests in GitHub actions (only my account can do it currently)

We still are working on making them actually report back to the PR properly

https://github.com/joshzcold/Friendly-Feud/actions/runs/12946026168/job/36109725655

@theredwillow
Copy link
Contributor Author

That's what I figured, but I didn't want to make an assumption and have a hanging PR if I was wrong.

You'd think that GitHub's UI would indicate whether something is a pipeline trigger.

@joshzcold
Copy link
Owner

This is great stuff! Something I always wanted to do the admin

I will just need some time to launch it and test out stuff that maybe the e2e didn't test (which your branch passed)

@joshzcold joshzcold requested a review from karlromets February 2, 2025 20:57
Copy link
Owner

@joshzcold joshzcold left a comment

Choose a reason for hiding this comment

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

@theredwillow can you resolve the conflict with master?

After that e2e tests still pass then we can merge.

Copy link
Collaborator

@karlromets karlromets left a comment

Choose a reason for hiding this comment

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

otherwise all good

Tests have also considerably changed in master, so make sure tests pass after resolving conflicts with AdminPage.jsx

send({ action: "data", data: game });
send({
action: "mistake",
data: game.teams[team].mistake,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not something you messed up, just noticed this. Is my assumption correct?

Suggested change
data: game.teams[team].mistake,
data: game.teams[team].mistakes,

@theredwillow
Copy link
Contributor Author

I have been in a huge move and finally got my desk set-up, so I needed to refresh my memory of how to get into this whole WSL/docker stuff. Then I needed to figure out the merge conflicts... which would have been nearly impossible with the traditional git diff checker, so I went through AdminPage's history and made the changes from these commits (the changes since Karl merged master into it)...

However, now I'm having trouble getting it to run make dev because of this issue which I don't understand.

image

@joshzcold
Copy link
Owner

joshzcold commented Feb 12, 2025

Hmm looks unrelated to your code changes.

I'll see if I can reproduce it locally.

The automatic e2e tests says the branch is looking good so we probably can merge before figuring out why you can't pull the nodejs alpine image.

Update: works on my machine, so its probably a temporary issue in the alpine repos and trying again will probably work for you.

Copy link
Owner

@joshzcold joshzcold left a comment

Choose a reason for hiding this comment

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

Currently answering final round questions is off by an index.
image

@joshzcold
Copy link
Owner

I think you should merge in the current e2e folder into your upstream so that you have the new tests that might catch an issue like I posted above.

@theredwillow
Copy link
Contributor Author

I think you should merge in the current e2e folder into your upstream so that you have the new tests that might catch an issue like I posted above.

Is this on a different branch? Are these files untracked? If I'm following upstream, why would those be missed?

@joshzcold
Copy link
Owner

Maybe the problem was local. I need to check my clone of your fork was up to date.

@theredwillow
Copy link
Contributor Author

My previous problem went away, but now I have this.
image

StackOverflow says to run npm cache clear --force, but I don't know how this is ran in all of this docker stuff.

I tried adding it to the front of dev-down in the Makefile, but it's not working.

theredwillow@DESKTOP-TGSPDI3:~/Destiny-Family-Feud$ sudo make dev-down
/bin/bash: line 1: npm: command not found
WARN[0000] Warning: No resource found to remove for project "famf".

@joshzcold
Copy link
Owner

Yeah, my bad. I was on master of your fork and not your branch.

Testing your branch now

Copy link
Owner

@joshzcold joshzcold left a comment

Choose a reason for hiding this comment

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

Great job 👍
Tried everything out and I am happy with the results.

Copy link
Owner

@joshzcold joshzcold left a comment

Choose a reason for hiding this comment

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

Sorry last second.
Found an error while trying logo upload

<label htmlFor="logoUpload">
<FileUp className="cursor-pointer text-secondary-900 hover:text-secondary-500" size={38} />
</label>
<input
Copy link
Owner

Choose a reason for hiding this comment

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

Just found an error on title logo upload
image

Copy link
Owner

Choose a reason for hiding this comment

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

Im not sure I understand the error since AfterUpload looks the same as BeforeUpload in the way it does it render method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to help, but I'm still stuck with the aforementioned Docker npm bug. I can't even access it to debug it anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but I don't know how this is ran in all of this docker stuff.

Just add it as a command, similar to how npm install is included.

For example:
image

Also purely guessing from the errors and the context:

  • 1st error seems to be related to TLS and bad MAC record (network issue?)
  • 2nd error seems to be again, related to a network issue — SSL cipher error
  • you recently moved (network changes?)

So very well could be something to do with the new network you are on...

Other suggestions for this error I saw:

  • simply restarting — WiFi on/off
  • npm cache verify

Try these both outside of Docker and within the Docker build to see if they make a difference.

import { useTranslation } from "react-i18next";
import { Buffer } from "buffer";
import { FileUp } from "lucide-react";
import { Image } from "next/image";
Copy link
Owner

Choose a reason for hiding this comment

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

@theredwillow the fix to the react render error is

Suggested change
import { Image } from "next/image";
import Image from "next/image";

There is a some detail in the Image component that requires it to be an import of "default export"
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import

Don't understand all of it, but this fixes the LogoUpload.

If you make this fix I can test everything out and we can move on 👍

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.

3 participants