Implement change alerts on Manage Queues and Queue Manager pages (#157, #160, #163) #258
Conversation
ssciolla
added
enhancement
|
@jlost, I think this is functional, but it may be too ambitious to try to squeeze it in this release. I thought I'd start the conversation about the code though so we can keep it moving forward. |
src/assets/src/changes.ts
Outdated
|
|
||
| // https://lodash.com/docs/4.17.15#xorWith | ||
|
|
||
| export function compareEntities<T extends Base> (oldOnes: T[], newOnes: T[]): string | undefined |
There was a problem hiding this comment.
Base can be a lot of things, but only the following types are supported in this function: Meeting, Queue. You could tighten up the typing here by further constraining your generic constraint to T extends Meeting|QueueBase.
There was a problem hiding this comment.
You could also say type ComparableEntity = Meeting | QueueBase then make this function non-generic, replacing T with ComparableEntity. The outcome would be the same. When I see a function that uses a generic type, my assumption is that it doesn't make any attempt to figure out anything else about the generic type's structure (IOW, as written I'd expect this function to care about properties in Base and nothing else). This comment could also sort of apply to the isUser checks in detectChanges, although that one is making an exception for a single type and is otherwise truly generic, so it's not as bad.
There was a problem hiding this comment.
The other direction you could go in would be making these functions even more generic, factoring out the type checks somehow. I find that recusively comparing JS objects to diff them is a problem I run into surprisingly often, which makes me wonder if there are libraries. A cursory google search shows there are some, but I haven't tried them yet. They might be worth considering. Otherwise, if they don't look appropriate or it would take too much work to pay off, I think this general approach is OK.
There was a problem hiding this comment.
Nice points, let me think this over and try out the changes you suggest.
There was a problem hiding this comment.
As far as packages, I've found:
- https://github.com/flitbit/diff
- https://github.com/cosmicanant/recursive-diff
- https://github.com/mattphillips/deep-object-diff
Did you see any others? The first looks fairly reliable, though it hasn't been updated since 2018, and doesn't have TypeScript support. The second is much less widely used. Looking at the third...
There was a problem hiding this comment.
You could also say
type ComparableEntity = Meeting | QueueBasethen make this function non-generic, replacingTwithComparableEntity. The outcome would be the same.
I might be betraying my hazy understanding of generics, but question: is there any merit to keeping the generic because otherwise the function could allow oldOnes and newOnes to be different types of objects if their types are each ComparableEntity[]? In other words, does keeping the generic somehow require that the function receive arrays of objects with the same type in these two parameters?
There was a problem hiding this comment.
Hard to say which lib is best without doing a deep dive, but I tend to gravitate toward things that have more users, stars, contributors, etc. and flitbit/diff seems to have quite a lead there. Its types seem to be in @types/deep-diff.
is there any merit to keeping the generic because otherwise the function could allow oldOnes and newOnes to be different types
Absolutely, good point, keeping it generic makes sure both params are the same type.
There was a problem hiding this comment.
Ah, I must have missed something regarding TypeScript and deep-diff. That looks promising; I will investigate that further. deep-object-diff was a little unpredictable, and wasn't giving me enough info.
Regarding your ComparableEntity suggestion and generics, see commit 1bd1d20 . I elected to keep Base in models.ts, but I can remove it entirely if you want.
…unce changes if detectChanges finds something
ssciolla
force-pushed
the
issues-157-160-change-event-alerts
branch
from
66da283 to
54f18fc
Compare
| @@ -5,6 +5,8 @@ | |||
| "@fortawesome/fontawesome-svg-core": "^1.2.28", | |||
| "@fortawesome/free-solid-svg-icons": "^5.13.0", | |||
| "@fortawesome/react-fontawesome": "^0.1.9", | |||
| "lodash.isequal": "^4.5.0", | |||
| "lodash.xorwith": "^4.5.0", | |||
There was a problem hiding this comment.
I'd also be OK with including all of lodash, since I happen to like it and we have no need to economize on bundle size.
There was a problem hiding this comment.
Sure, I'll get to this.
| <Col md={12}> | ||
| <ChangeLog changeEvents={props.meetingChangeEvents} deleteChangeEvent={props.deleteMeetingChangeEvent} /> | ||
| </Col> | ||
| </Row> |
There was a problem hiding this comment.
Not really a technical comment but a stray observation - since these are positioned toward the top, the rest of the page shifts downward a bit when they appear, which might cause misclicks. Not sure what the best way to handle that would be.
There was a problem hiding this comment.
Hm, yeah, tricky, it seemed like a simpler implementation to just always report them at the top, but they could be hidden from view or result in some page shifting if there are a lot of meetings in play.
ssciolla
force-pushed
the
issues-157-160-change-event-alerts
branch
from
9172e52 to
ba5614e
Compare
This PR introduces a front-end mechanism for tracking changes to certain entity types (so far,
QueueBase,Meeting) that are updated by WebSocket connections. These changes seek to alert all users (including those with vision impairment) that contents of pages have changed without their doing anything, as well as provide confirmation in some cases that certain actions were successful (specifically, with meeting actions on theQueueManagerPage). Alerts are set to be visible for seven seconds before disappearing. The PR aims to resolve issues #157, #160, and #163.