Adds ability to request inventory report. (PP-1236) #110
Conversation
| ? UNKNOWN_COLLECTIONS_MESSAGE | ||
| : collections.length === 0 | ||
| ? NO_COLLECTIONS_MESSAGE | ||
| : SOME_COLLECTIONS_MESSAGE; |
There was a problem hiding this comment.
I'm not a fan of nesting ternary operators. An extra level of indent on the inner branches might help.
There was a problem hiding this comment.
I'm pretty sure I had this indented. I think eslint undid it. I'll try again.
There was a problem hiding this comment.
Yeah, eslint is doing this in the pre-commit hook.
I must admit that I am a fan of the ternary, even in its nested form and I prefer it over if/else if/else and switch/case alternatives in this scenario. It makes it super clear that one thing is getting assigned/returned here and that there is nothing else to keep track of.
| : SOME_COLLECTIONS_MESSAGE; | ||
| const collectionList = (collections: InventoryReportCollectionInfo[]) => { | ||
| return ( | ||
| collections?.length > 0 && ( |
There was a problem hiding this comment.
I had to look up what happens when you compare null/undefined with 0, and this does in fact work, but you could also remove > 0, since null, undefined, and 0 are all falsy. That might be less alarming.
| />, | ||
| { queryClient } | ||
| ); | ||
| container.querySelector(".modal-content"); |
There was a problem hiding this comment.
Does this do anything? It's in some of the tests.
There was a problem hiding this comment.
Good catch. No, it wasn't doing anything. I removed it. I was able to do what I needed with getAllByRole and queryAllByRole on the "dialog" role.
| const heading = within(modalContent).getByRole("heading", { | ||
| level: MODAL_HEADING_LEVEL, | ||
| }); | ||
| const modalBody = modalContent.querySelector(".modal-body"); |
There was a problem hiding this comment.
I feel like there should be a better (more RTL) way than querying for the class name, but I'm not sure what would be an appropriate role. I guess you could do getByText instead, but I don't see an easy way to exclude the text from being in the heading, which maybe you want?
There was a problem hiding this comment.
Yeah, I was trying to distinguish this from the heading text. I used the more RTL queries whenever I could.
| { queryClient } | ||
| ); | ||
| container.querySelector(".modal-content"); | ||
| const modalContent = getByRole("document"); |
There was a problem hiding this comment.
It feels like getByRole("dialog") should be in here somewhere, since that would be the distinguishing characteristic of what you're looking for. But I don't know if react-bootstrap is setting that role.
There was a problem hiding this comment.
I did try using that initially, but it wasn't working, so I took a different tack. I did manage to sort out some of the other problems I was having, so I'll take another look at it.
There was a problem hiding this comment.
The issue is that there are two elements with the "dialog" role, which I had somehow missed before. I switched to getAllByRole and queryAllByRole to resolve this and streamlined the tests in the process.
Description
react-query(@tanstack/react-query) to the project. This will give us a path away from Redux and allow us to start making the distinction between client app state and server synchronization.componentWithProvidersandrenderWithProvidershelpers to make it easier to test with all of of our providers. It can be extended to include our Redux provider, once it's needed.Motivation and Context
We need a way for users to trigger the inventory reports now available via the backend.
[Jira PP-1236]
How Has This Been Tested?
Checklist: