-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Alert screen for CODIS operational room #42
Conversation
Thanks for the PR, will review it asap 👍 |
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 a lot for this PR, overall great maybe adding 2 screenshots of how the 2 big screens renders would be nice :)
78dea22
to
4b4c25a
Compare
Thank you for your review @Akilditu ! I added two screenshots in the description of the PR 👌 |
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 changes and answers !
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.
Hello!
Thanks a lot for this PR, it clarifies a lot of things and the alert screen seems to work perfectly 😊
I have added a few very dumb comments regarding naming / styling, I hope they are not too "relous". However, there is maybe a more substantial discussion regarding the three different callbacks being triggered by the same input (Input("interval-component-homepage", "n_intervals")
). Let me know what you think about this!
Thank you @pechouc for your review ! Mostly I agree with you (I even think that to improve the quality of the code, we should do some refactoring. But it is clearly not the priority). Regarding the merge of |
3839b4b
to
5f9c20d
Compare
@Akilditu @pechouc I had to remove the following lines in
Many users had the same issue which has been reported to the creators of CSSLint, see this issue on Github. Let's hope that our users won't have IE < 10: the blink features won't work for them ! |
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 a lot for the changes, I agree with you: let's keep the callbacks as in the revised version! As you rightly mentioned, this logic will probably change significantly with the web-hook.
Motivation
Hi all ! This PR aims at transforming our platform into a multiple page app:
As discussed, only the lastest event (as latest fire) is displayed on the screen. When multiple alerts are given by the RPI for the same event (same fire), we display a GIF of all images incoming (doubt relief for the firefighters).
Features
Multiple features have been added:
Changes in rough layout:
div_live_alerts_data
: in order to store the table of live alerts we get once the call to the API is donelast_displayed_event_id
: to keep track of the last event (fire) which has been displayed on the alert screen (useful for the GIF part)images_url_current_alert
: this is a Dict[str, List[str]]. Keys: str of the event id. Values: List of all the urls which belong to this event id (again for GIF part)This might change with the use of a webhook, but we will still need to store the data somewhere to make it accessible to multiple callbacks.
Limits
As we know, the webhook will change many things, therefore some developments made here might become outdated. Also we will need to think about deleting some info at some point, especially for the Store
images_url_current_alert
, otherwise the dict will keep on growing. Once a event has been "acquitté", we can remove it from the dict. This will be easy but we need to wait for the webhook to get a better picture of the workflow needed.Screens
When there are no alerts:
When there is an alert: