-
Notifications
You must be signed in to change notification settings - Fork 54
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
Team logos #529
Team logos #529
Conversation
Someone is attempting to deploy a commit to a Personal Account owned by @evroon on Vercel. @evroon first needs to authorize it. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #529 +/- ##
=======================================
Coverage 90.45% 90.45%
=======================================
Files 121 123 +2
Lines 3875 3938 +63
=======================================
+ Hits 3505 3562 +57
- Misses 370 376 +6 ☔ View full report in Codecov by Sentry. |
Yeah I don't understand the tests. Erik you'll have to help me on this. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@robigan The tests are already correct right? |
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.
Thanks for this! LGTM overall, have a few minor suggestions.
Also I think this branch has to be rebased now on master before it can be merged.
backend/bracket/routes/teams.py
Outdated
assert extension in (".png", ".jpg", ".jpeg") | ||
|
||
filename = f"{uuid4()}{extension}" | ||
new_logo_path = f"static/{filename}" if file is not None else None |
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 it would be better to store it in a subdirectory team_logos
actually, I should have done that too for the tournament logos
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.
Do you want me to store the tournaments logos in a subdir too? I assume that's what you want so I'll commit that in if you don't reply to this before I make the change.
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.
Btw, I won't use team_logos
but rather team-logos
as REST generally tends to prefer -
over _
.
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.
Do you want me to store the tournaments logos in a subdir too?
I was hesitant because it's a breaking change but let's just do it
handleRequestError(response); | ||
const response = await useUploadLogo(files[0]); | ||
await swrResponse.mutate(); | ||
handleRequestError(response as unknown as AxiosError); // TODO: Check with Erik if this is correct |
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.
Can't it be handleRequestError(response as AxiosError);
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.
Nope, TypeScript complains about the insufficient overlap. I just added type defs to the axios library as it was annoying me that there weren't any. Also why is Axios required rather than imported?
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.
Also why is Axios required rather than imported?
Ah no particular reason, must be an oversight.
Awesome work, thanks! |
Add team logos.
@evroon @Sevichecc can you check translations are correct in your languages? I noticed chinese translation is missing some entries btw.
closes #515