-
Notifications
You must be signed in to change notification settings - Fork 241
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
(fix) O3-3199: Service Queues - queue table not updated after transitioning a queue entry #1178
Conversation
Size Change: -51.8 kB (-1.47%) Total Size: 3.47 MB
ℹ️ View Unchanged
|
@@ -41,7 +41,7 @@ describe('Home Component', () => { | |||
window.location = { pathname: '/some-path/screen' }; | |||
|
|||
render(<Home />); | |||
expect(screen.getByRole('progressbar')).toBeInTheDocument(); | |||
expect(screen.getByText(/patients currently in queue/i)).toBeInTheDocument(); |
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.
@donaldkibet I don't really know why this test broke. Could you confirm that I'm not breaking anything that shouldn't be broken?
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.
Ok, I'll take the thumbs up as meaning "this change seems fine."
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.
Looks reasonable enough, but I'd really want to do some testing myself before approving it. I wouldn't be surprised if there is an off-by-one error either ':-)
if (data && data.length && data?.[data?.length - 1]?.data?.links?.some((link) => link.rel === 'next')) { | ||
// Calling for the next set of data | ||
setSize((prev) => prev + 1); | ||
const nextUrl = getNextUrlFromResponse(pageData); |
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.
indent
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.
?
Prettier handles things like indentation so we don't have to think about it (and so it can't be wrong)
Co-authored-by: chibongho <cbho@pih.org>
@chibongho Per our conversation, I have made it so that tables show data as soon as they get that data, even if more data is still loading. Demo with limit=3 recording-2024-06-12_13.10.07.mp4 |
I'll fix the E2E tests soon. Apparently Playwright doesn't support non-LTS Ubuntu, which is nuts. So I guess it's time to upgrade to 24.04. |
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.
Looks great! thanks.
The E2E tests are failing due to https://openmrs.atlassian.net/browse/O3-3401 |
Requirements
Summary
useSWRInfinite
does not work withmutate
. So I implemented a custom paging SWR hook that supports mutation. I tried writing tests and failed. If we eventually decide to turn this into a more generic paging data hook, we will need to write tests. Also, it retains the original behavior of just downloading all the data at once, iteratively downloading every page.Screenshots
recording-2024-06-06_16.36.51.mp4
Related Issue
https://issues.openmrs.org/browse/O3-3199
Other