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

Created event for ScheduledTimeChanged and added snackbars based on that #897

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

TheCommandCat
Copy link
Contributor

Closes #849

Hey, Apologies for the new PR - I accidentally deleted the branch while trying to upload the updated code, which caused the first PR to close.

Based on the feedback from @johnmeshulam, I've added a new event specific to schedule changes and included snackbars for the Field Manager and Judge Advisor when the event is emitted.

While working on this, I started wondering if it might be useful to include more context about the specific changes in the schedule (e.g., by passing data through the event and displaying it in the snackbars). Do you think this level of detail would be helpful for these roles, or is the current approach good enough?

Copy link
Member

@johnmeshulam johnmeshulam left a comment

Choose a reason for hiding this comment

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

Much better, added some comments regarding standardization and usability,

libs/types/src/lib/websocket.ts Outdated Show resolved Hide resolved
apps/frontend/pages/lems/judge-advisor.tsx Outdated Show resolved Hide resolved
apps/frontend/pages/lems/judge-advisor.tsx Outdated Show resolved Hide resolved
apps/frontend/pages/lems/field-manager.tsx Outdated Show resolved Hide resolved
apps/frontend/pages/lems/field-manager.tsx Outdated Show resolved Hide resolved
apps/backend/src/websocket/handlers/field.ts Outdated Show resolved Hide resolved
apps/backend/src/websocket/handlers/field.ts Outdated Show resolved Hide resolved
apps/backend/src/websocket/handlers/judging.ts Outdated Show resolved Hide resolved
@TheCommandCat
Copy link
Contributor Author

Should the judge-advisor also receive an informational message instead of just 'הלוז עודכן'? If so, what format should the message be? @johnmeshulam

@johnmeshulam
Copy link
Member

Please see my previous comments. As I mentioned, the schedules time does not change

@TheCommandCat
Copy link
Contributor Author

TheCommandCat commented Jan 2, 2025

Im really sorry about all the mess...

I'm not entirely sure I understood your last comment. As I understand it, the schedule itself remains unchanged, and only the assigned teams are updated. Based on your advice, I renamed the events to more appropriate names. Is the issue with the specific message 'הלוז עודכן'? Should I update it to something like 'הקבוצה בסשן השיפוט בשעה {time} עודכנה' instead?

If yes, should i pass the updated JudgingSession object in the judgingSessionTeamUpdated event as well like I did previously with the field manager to get the JudgingSession info for the snackbar?

@johnmeshulam
Copy link
Member

The logic will not work as it's written right now. It seems like you understood eerything correctly but the logic itself is flawed, if I'll have time l'll leave more detailed comments later

@TheCommandCat
Copy link
Contributor Author

Hey, I just pushed a small fix for a mistake I made (apologies for that). However, there are still two things that could be improved:

  1. The snackbars don’t adjust their size based on the message length, which sometimes causes the message to overlap. I couldn’t find an easy fix for this in the Notistack docs, other than overriding the CSS.
  2. I thought it would be helpful to include the room name in the judgment update, but I didn’t find an easy way to retrieve it. The JudgingSession object only contains the room ID. Should we pass the room name with the backend event?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Under Review
Development

Successfully merging this pull request may close these issues.

Add snackbars for schedule changes
2 participants