(feature) O3-2750 Service Queues - Configurable queues table to display queue entries by status #968
Conversation
…ay queue entries by status
chibongho
requested review from
mseaton,
mogoodrich,
denniskigen,
jecihjoy,
CynthiaKamau,
donaldkibet and
ibacher
|
Size Change: -53.4 kB (-2%) Total Size: 2.89 MB
ℹ️ View Unchanged
|
|
|
||
| const apiUrl = `/ws/rest/v1/visit-queue-entry?location=${queueLocationUuid}&v=full`; | ||
| const { t } = useTranslation(); | ||
| useTranslation(); |
| ?.map(mapVisitQueueEntryProperties) | ||
| .filter((data) => data.service == currServiceName); | ||
| const mappedVisitQueueEntries = visitQueueEntries?.map(useVisitQueueEntryProperties); | ||
| if (currServiceName && currServiceName !== t('all', 'All')) { |
There was a problem hiding this comment.
On the other hand, it looks like we need the translation hook here?
There was a problem hiding this comment.
Good catch. Thanks. Missed this when I was refactoring the function. I'm surprised this didn't given me any errors.
There was a problem hiding this comment.
seems odd that we are basing logic on string comparisions to a localized string, but I suspect this is the existing logic, not something you are introducing?
There was a problem hiding this comment.
correct. This is old code.
| _type: Type.Array, | ||
| _description: 'Columns to appear in the queues table.', | ||
| _elements: { | ||
| id: { |
There was a problem hiding this comment.
Do we need this property? In all of the examples, it ends up being the headerI18nKey...
There was a problem hiding this comment.
Yeah, we can forgo this. This means requiring the columns headers to be unique, which should be fine.
| export const queueTableName = getSyncLifecycle(QueueTableName, options); | ||
| export const queueTablePriority = getSyncLifecycle(QueueTablePriority, options); | ||
| export const queueTableStatus = getSyncLifecycle(QueueTableStatus, options); |
There was a problem hiding this comment.
Use getAsyncLifecycle(). getSyncLifecycle() should basically just be used for links.
|
|
||
| const currentStatusIndex = Math.max(0, allowedStatuses?.findIndex((s) => s?.uuid == currentStatusUuid)); | ||
|
|
||
| const visitedQueueEntriesByLocationAndQueueAndStatus = visitQueueEntriesByLocation |
| </TabList> | ||
| <TabPanels> | ||
| {allowedStatuses?.map((s) => ( | ||
| <TabPanel key={s?.uuid} style={{ padding: 0 }}> |
There was a problem hiding this comment.
We shouldn't use something as a key if it may be undefined. It would be better to use the index argument, if this may not be set.
There was a problem hiding this comment.
When would it be null? I'd think we can just remove the ?
There was a problem hiding this comment.
Sorry, that was sloppy on my part. It should never be undefined/null. I'll remove the ?. I think using index as key is bad unless the same index always corresponds to the same object (which is not the case here).
There was a problem hiding this comment.
This does not appear to be fixed yet.
| selectedIndex={currentStatusIndex} | ||
| onChange={({ selectedIndex }) => setCurrentStatusUuid(allowedStatuses[selectedIndex].uuid)} | ||
| className={styles.tabs}> | ||
| <TabList style={{ paddingLeft: '1rem' }} aria-label="Outpatient tabs" contained> |
There was a problem hiding this comment.
It would be better to define a scss file and put this in a class (this should also be a standard Carbon spacing unit).
| paginatedQueueEntries?.map((queueEntry) => { | ||
| const row = {}; | ||
| queueTableColumns.forEach((conf) => { | ||
| row[conf.id] = <ExtensionSlot name={conf.extensionSlotName} state={{ queueEntry }} />; |
There was a problem hiding this comment.
Yikes! This is stress-testing the extension system! I was imagining this working as something like a slot-per-column, or, better yet, a simple mapping function rather than a slot-per-cell. I guess we'll see how this works...
There was a problem hiding this comment.
For reference, a 20-person queue with just 3 columns is now 60 separate slots with 60 separate extensions.
There was a problem hiding this comment.
Is that true? If so, I agree we may need an alternative. Ultimately, we just want to be able to have a list of columnDefinitions that can be configured with a value function or component that takes in a queueEntry as an argument/prop. If there was a straightforward way to register functions or components with a lookup key, and then to refer to these with this key to use when rendering, that's what we need.
It isn't entirely similar, but I'd think this kind of thing could also eliminate the kind of hard-coding that one finds in places like this and this. The similarity is that there is a need to support end-user configuration via a string value, and then have this string value determine what actual component is rendered.
There was a problem hiding this comment.
@ibacher I did some rough profiling work. Using a table with 10 rows and 3 columns on my computer, I measure the time to render the cells using the components directly vs using <ExtensionSlot> . The direct way has 1ms of time lapse between the useEffect(..., []) calls of the first and the last rendered cells, while via <ExtensionSlot> the time lapse is 20ms. So... technically 20x worse, but still small enough to be acceptable?
There was a problem hiding this comment.
Just seeing this thread now, it wasn't visible when I was looking at the PR itself... did we move away from this approach? I agree that having a slot-per-cell seems like a lot, but don't necessarily want to push for a full refactor now if we want to just see how it works in this case.
One goal on my end would be keep the code a simple as possible (the key being "as possible", I know there will be some complexity) from a extension and readability perspective for what I suspect will be the 80% use case for most implementions, just to use the default columns.
There was a problem hiding this comment.
I don't love this approach., but I'm keeping it as is for now (unless others have objection). It's ok performance-wise, but developer experience isn't great as we need somewhat bloated JSON to define the columns. This approach also doesn't address how to go about having the table show different columns at different places. (For example, we might want a "current queue" column when we are looking at queue entries from all queues, but can hide the column if we're only looking at a specific queue.)
There was a problem hiding this comment.
@chibongho and I talked more about this today, and we decided we are going to forego the config-based column configurability and move that into a separate draft PR for that POC work, in favor of moving to an approach that allows configurability via props and re-usable components. Stay tuned.
| }; | ||
|
|
||
| export const QueueTablePriority = ({ queueEntry }: { queueEntry: VisitQueueEntry }) => { | ||
| return <>{queueEntry.queueEntry.priority.display}</>; |
There was a problem hiding this comment.
This component really should render a pill, as shown in the designs.
There was a problem hiding this comment.
I'll make them pills. We might have to re-think the coloring if we want the flexibility of having available priorities configurable per queue (currently in our spec). I'll worry about that in a later PR.
There was a problem hiding this comment.
Yeah, my original thought was to have the color based a ConceptAttribute, so the queue priorities could tell you how they should appear. Thinking particularly about, e.g., color-based triage systems.
| @@ -119,10 +120,10 @@ export const QueueTableByStatus: React.FC = () => { | |||
| }; | |||
|
|
|||
| interface QueueTableProps { | |||
| visitedQueueEntriesByLocationAndQueueAndStatus: VisitQueueEntry[]; | |||
| visitQueueEntriesByLocationAndQueueAndStatus: VisitQueueEntry[]; | |||
There was a problem hiding this comment.
I would expect a QueueTable to take in a queueEntries or visitQueueEntries property. Why are we calling these things ByLocationAndQueueAndStatus? Ideally we'd use the same QueueTable and QueueTable props to render QueueTables in different contexts, with different columns, and with different collections of QueueEntries. In the context above, you might set the queueEntries from visitQueueEntriesByLocationAndQueueAndStatus, but that is only this particular use case.
There was a problem hiding this comment.
I wanted to make it explicit that this VisitQueueEntry[] has been filtered. You're right that the filtering can be in different contexts and does not necessarily have to be by Location/Queue/Status. I can renamed it to queueEntriesToDisplay.
There was a problem hiding this comment.
queueEntriesToDisplay is fine I guess, but I still prefer just queueEntries for readability and consistency. I don't think anyone should have any expectation that this is every queue entry since the beginning of time that is expected to be passed in as a prop. If this was always going to have the same usage, it wouldn't be a prop, you would just retrieve them within the component. Passing it as a prop implies that the queue table takes in a list of queue entries to render, as that's the whole point.
There was a problem hiding this comment.
If I'm following correctly :) I agree with Mike's logic here.
| </TabList> | ||
| <TabPanels> | ||
| {allowedStatuses?.map((s) => ( | ||
| <TabPanel key={s?.uuid} style={{ padding: 0 }}> |
There was a problem hiding this comment.
This does not appear to be fixed yet.
| return <>Loading....</>; | ||
| } else if (noStatuses) { | ||
| return <>No available statuses configured for queue</>; |
There was a problem hiding this comment.
All of these messages need to be translated, not hard-coded in English
| featureName: 'queue table name column', | ||
| moduleName, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Can you explain why this has been modified like this, instead of just changing "getSyncLifecycle" to "get AsyncLifecycle"?
There was a problem hiding this comment.
The two functions don't take in the same inputs. getAsyncLifecycle expects a Promise returned by dynamically calling import("***.component"), where ***.component.tsx is usually a file that defines a Component that is exported with default export'. In this case, queue-table-cells.component.tsx has several exports, and therefore no default export, The .then(...) syntax is admittedly uglier than the pattern in other places. The alternative would be to create separate files for each of those 3 components defined in queue-table-cells.component.tsx.
There was a problem hiding this comment.
OK, defer to others like @ibacher on this. I don't feel like I tend to see it like this in the code elsewhere, so that might imply that the right thing is to move the components into separate files with default exports?
There was a problem hiding this comment.
Yeah, generally, the component should be the default export, even if other things are exported (although that sounds suspiciously like code that shouldn't all be in one component file; usually a component file should just define a single React component and if things need to be shared, they go elsewhere).
There was a problem hiding this comment.
Yeah, I need to refresh my memory on how all this stuff works, more or less defer to @ibacher on this one
| <QueueTable | ||
| visitedQueueEntriesByLocationAndQueueAndStatus={visitedQueueEntriesByLocationAndQueueAndStatus} | ||
| /> | ||
| <QueueTable visitQueueEntriesByLocationAndQueueAndStatus={visitQueueEntriesByLocationAndQueueAndStatus} /> |
There was a problem hiding this comment.
The entries we are passing in here have nothing to do with ByLocation, do they? Filtering by Queue and Status is enough. I don't believe we need to do anything with Location?
There was a problem hiding this comment.
Right now, I'm doing useUnmappedVisitQueueEntries(currentLocationUuid), which is only getting visitQueueEntries from the current location (hard coded to user's session location right now). I haven't done this yet, but I plan to have the list of queues dropdown (from useQueues()) be limited by current location as well, and hook up the location dropdown currently defined in the queue app's home page to control this location.
There was a problem hiding this comment.
The whole "mapped" vs "unmapped" visit queue entries thing is a bit confusing to me, to be honest. Do we need to have both of these? I understand the value in having a nicer object to work with than the REST response, but this doesn't feel exactly right. What is done in other esms that have user-friendly objects to work with and map to/from rest representations?
Regarding the other point, this should really depend on the nature of the overall page that is using this component. I would not expect variable names (or assumptions) like this within a "queue-table" component. If there is a page called "queues-for-location", then it would make sense to have variable names in that page that have names like that. But in a generic queue table component - no.
There was a problem hiding this comment.
I intend to get rid of the MappedVisitQueueEntry when this new table is more feature complete and can replace the existing table. The existing table uses MappedVisitQueueEntry, so for now supporting both the unmapped and mapped versions is meant to be an un-invasive way to reuse old code while we try to migrate. I can add some comment about that. (@mogoodrich is also getting rid of the similar MappedAppointment type in his other PR)
| @@ -105,8 +107,8 @@ export interface MappedVisitQueueEntry { | |||
| queueComingFrom: string; | |||
| } | |||
|
|
|||
| interface UseVisitQueueEntries { | |||
| visitQueueEntries: Array<MappedVisitQueueEntry> | null; | |||
| interface UseVisitQueueEntries<T extends MappedVisitQueueEntry | VisitQueueEntry> { | |||
There was a problem hiding this comment.
I haven't dove deeplying the queue module yet, so I'm just guessing about this, but if this is anything like the Appointment module, do we want to try to standardized on a single queue entry interface (probably VisitQueueEntry and retire the MappedVisitQueue entry?). Is a lot of this code just stuff to support translating between a mapped and "normal" visit queue entry? (The answer may be yes, but we don't want to tackle it in this commit).
There was a problem hiding this comment.
Update: I see the later comments about getting rid of this, +1
| ?.map(mapVisitQueueEntryProperties) | ||
| .filter((data) => data.service == currServiceName); | ||
| const mappedVisitQueueEntries = visitQueueEntries?.map(useVisitQueueEntryProperties); | ||
| if (currServiceName && currServiceName !== t('all', 'All')) { |
There was a problem hiding this comment.
seems odd that we are basing logic on string comparisions to a localized string, but I suspect this is the existing logic, not something you are introducing?
| featureName: 'queue table name column', | ||
| moduleName, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Yeah, I need to refresh my memory on how all this stuff works, more or less defer to @ibacher on this one
| @@ -119,10 +120,10 @@ export const QueueTableByStatus: React.FC = () => { | |||
| }; | |||
|
|
|||
| interface QueueTableProps { | |||
| visitedQueueEntriesByLocationAndQueueAndStatus: VisitQueueEntry[]; | |||
| visitQueueEntriesByLocationAndQueueAndStatus: VisitQueueEntry[]; | |||
There was a problem hiding this comment.
If I'm following correctly :) I agree with Mike's logic here.
|
Ok, updated the PR.
|
Requirements
Summary
A new queues table with the following feature:
This table is a work in progress, and is mounted on an alternate route (/queue-table-by-status). I intend for this table to replace the existing table in the main route (/) when it is feature-complete.
Screenshots
OpenMRS.5.-.-.Microsoft.Edge.2024-02-06.15-43-34.mp4
Related Issue
Other