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

Fixing "next match" feature #1498

Open
wants to merge 27 commits into
base: dev
Choose a base branch
from
Open

Fixing "next match" feature #1498

wants to merge 27 commits into from

Conversation

varCepheid
Copy link
Collaborator

@varCepheid varCepheid commented Apr 8, 2022

closes #1493
At the top of the event and event-team pages, instead of one "next match," the page shows the three most relevant upcoming matches.

@varCepheid varCepheid requested a review from calebeby April 8, 2022 00:33
@varCepheid varCepheid self-assigned this Apr 8, 2022
@calebeby
Copy link
Member

calebeby commented Apr 8, 2022

Hey @varCepheid, usually the scheduled match time and the predicted match time are wrong by 5-10 minutes. Do you think that instead of this solution we could show "upcoming matches" as 2-3 matches?

… before and three after the current time; in fixing some aestethic issues a couple more popped up, but I don't know exactly how to fix those (and they're a bit less noticeable)
@ev118 ev118 self-assigned this Apr 11, 2022
@ev118
Copy link
Contributor

ev118 commented Apr 12, 2022

I think the problem has been solved, though a bit of tweaking may be useful so that it looks neat (the Upcoming Matches header is off-center, even though it says justify-self: center; and the search bar sits right on top of Qual 1).

@varCepheid varCepheid removed their assignment Apr 12, 2022
@calebeby
Copy link
Member

Hey @ev118, thanks for updating this! I think there are several opportunities to simplify the approach here, would you be open to scheduling a meeting this weekend to work through that with me?

@varCepheid varCepheid added bug Addresses something that isn't working. needs work Changes have been suggested and need to be implemented. labels May 9, 2022
@varCepheid varCepheid marked this pull request as draft June 6, 2022 04:41
@varCepheid varCepheid marked this pull request as ready for review February 17, 2023 20:55
@varCepheid varCepheid removed the needs work Changes have been suggested and need to be implemented. label Feb 17, 2023
@calebeby
Copy link
Member

calebeby commented Aug 9, 2023

@varCepheid can you delete commented-out code from this PR? Generally it is good practice to try to avoid committing commented-out code to dev/main branches.

@calebeby calebeby added the needs work Changes have been suggested and need to be implemented. label Aug 9, 2023
@varCepheid varCepheid removed the needs work Changes have been suggested and need to be implemented. label Nov 9, 2023
@varCepheid
Copy link
Collaborator Author

Can you delete commented-out code from this PR? Generally it is good practice to try to avoid committing commented-out code to dev/main branches.
@calebeby Done.

src/routes/event-team.tsx Outdated Show resolved Hide resolved
@calebeby calebeby added the needs work Changes have been suggested and need to be implemented. label Nov 12, 2023
<>
<h2 class={sectionStyle}>Next Match</h2>
<MatchDetailsCard match={nextMatch} eventKey={eventKey} link />
<Heading level={2}>Upcoming Matches</Heading>
Copy link
Member

Choose a reason for hiding this comment

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

This is a little nitpicky, but the space between the "upcoming matches" heading and the actual match cards is half the size of the space between the other elements on the page.

Before this PR:

image

After this PR:

image

The new heading looks nice in terms of the underline and text styling, but it feels a little bit cramped without the even space around it.

@calebeby
Copy link
Member

@varCepheid I also noticed that the issue #1493 is kind of vague and doesn't mention whether it is talking about the next match feature on the team@event page or on the event page. This PR only fixes the team@event page, so maybe we should leave that issue open when this is merged and open another PR for the event page.

removed "No Upcoming Matches" on event-team page
added space below header
changed getUpcomingMatches to new series of filters
@varCepheid
Copy link
Collaborator Author

I agree that the issue is vague. I added a description on the PR, which is hopefully more clear. And the changes here do affect the event page; if it wasn't doing anything different, we should definitely fix that here.

@varCepheid varCepheid removed the needs work Changes have been suggested and need to be implemented. label Nov 15, 2023
@varCepheid varCepheid self-assigned this Nov 15, 2023
@varCepheid
Copy link
Collaborator Author

When I merged from dev, it brought a bunch of changes to package-lock.json. I tried to deal with all of them, but the file is so large that it was lagging a lot, and I think I made a mistake somewhere. Should I try to revert the commit?

@calebeby
Copy link
Member

@varCepheid sorry for not getting back to you sooner. You shouldn't ever need to manually merge a package-lock.json file. You can always just checkout the package-lock.json from either side of the merge, and then run npm i to make npm fix any issues or discrepancies from the package.json file. In this case, there are no changes to package.json so we don't even need to do that. Since the changes to package-lock.json aren't relevant to this PR, I went ahead and reverted them. (git checkout origin/dev -- package-lock.json is the easiest way to do this for a single file without necessarily reverting an entire commit, it just means "set this file to as it was on _ branch").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Addresses something that isn't working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Next match" feature isn't always accurate
3 participants