-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
Report #1469
base: master
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ef1f200
to
b69c374
Compare
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 this is looking really good already and I am excited for this feature.
Great job with all the work to get this far
I would suggest that we simplify the code and consider the report as part of the user flow
I wonder if:
- User starts the app and clicks the
Start
->Next
button to go through the interface. lets call this theprogress button
just so we have a reference - When the user is on the last event, the
progress button
button showsEnd show
. Clicking end show will generate a report and terminate things - while we have a report, the
progress button
showsClear report
. Clicking here will clear the report and the button will then showStart
Also, is it now the time for us to consider the rehearsal mode? (ie: offsets would ignore schedule and only care about the event duration)
refetchInterval: queryRefetchIntervalSlow, | ||
networkMode: 'always', |
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.
question: I wonder if we prefer fetching this on demand?
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.
yeah I think that would be better, I just can't quite figur out how it ties togetter
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.
kinda like this
TanStack/query#1205
remember we have the messages from the server, perhaps in the ontime-refetch we could add one more case for the report?
}, []); | ||
|
||
return ( | ||
<Panel.Section> |
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.
question: I wonder if we should leave this for later once we have an export feature?
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.
we can do that
Was just thinking that this would also where you went to download the report
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 agree, we can add this UI when we have the download report feature
apps/client/src/features/rundown/event-block/composite/EventBlockChip.tsx
Outdated
Show resolved
Hide resolved
apps/client/src/features/rundown/event-block/composite/EventBlockChip.tsx
Outdated
Show resolved
Hide resolved
apps/client/src/features/rundown/event-block/composite/EventBlockChip.tsx
Outdated
Show resolved
Hide resolved
duration: number; | ||
} | ||
|
||
function EventReport(props: EventReportProps) { |
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 wonder if we can make this more efficient.
We know that, once an event has run, the report will never change until it runs again
Do you agree? if so, we can find a way together, to only run this once when the event finishes
//TODO: there seams to be some actions that should invalidate reports | ||
// events timer edits? | ||
// event delete | ||
// Also what about roll mode? |
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 there are too many edge cases here and we should keep it simple
The report, should tell the user how long did something take last time it ran
I dont think it should matter what playback mode it is in
As for clearing:
- The user would chose to clear the entire report
- The user deletes an event, so we can clear the report for it
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 dont think it should matter what playback mode it is in
I agree we just have to find all the places to hook in.
as for the clearing
the way we display the report right now it is relative to the events duration, so of the user changes the duration of the event, dose the report still make sense?
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 agree we just have to find all the places to hook in.
What if we use the same place where we do the integrations?
In that way, that becomes the side effects place
the way we display the report right now it is relative to the events duration, so of the user changes the duration of the event, dose the report still make sense?
I believe the report should be a feature that says "last time this ran, this is what happened", in which case it doesnt matter if the user changes things post fact. This also has advantage of keeping the feature simpler and a more straightforward mental model
apps/client/src/features/rundown/event-block/composite/EventBlockChip.module.scss
Outdated
Show resolved
Hide resolved
@@ -6,14 +6,18 @@ | |||
color: $label-gray; | |||
padding: 0.125rem 0.5rem; | |||
border-radius: 2px; | |||
|
|||
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.
tip: check your formatter, these spaces shouldnt be here
#1139
now
later ?