-
Notifications
You must be signed in to change notification settings - Fork 184
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
Add config options for default sorting/filtering values in variant analysis results view #2392
Conversation
// Without this await, `getByDisplayValue` could not find any selected dropdown option. | ||
await new Promise((resolve) => setTimeout(resolve, 0)); | ||
|
||
expect(screen.getByDisplayValue("With results")).toBeInTheDocument(); | ||
expect(screen.getByDisplayValue("Number of results")).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.
For some reason, await new Promise((resolve) => setTimeout(resolve, 0));
was necessary to get these "default" expectations to pass 🤔
Not sure why, but it works 😅 If anyone knows more about this, let us know!
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.
I'm not sure why this is happening (it could be that some of the VS Code UI toolkit shadow DOM is only rendered on the next tick or something like that), but a slightly more reliable way to solve this is by waiting for the element to start existing:
await waitFor(() => screen.getByText("With results"));
This way we don't depend on the exact tick and it will still fail if it doesn't find the element after some time.
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.
This is perfect! Thanks ✨
The default filterSortState is no longer to show "all" repositories, so I've created an explicit "all-inclusive" filterSortState for the purpose of these tests.
// Without this await, `getByDisplayValue` could not find any selected dropdown option. | ||
await new Promise((resolve) => setTimeout(resolve, 0)); | ||
|
||
expect(screen.getByDisplayValue("With results")).toBeInTheDocument(); | ||
expect(screen.getByDisplayValue("Number of results")).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.
I'm not sure why this is happening (it could be that some of the VS Code UI toolkit shadow DOM is only rendered on the next tick or something like that), but a slightly more reliable way to solve this is by waiting for the element to start existing:
await waitFor(() => screen.getByText("With results"));
This way we don't depend on the exact tick and it will still fail if it doesn't find the element after some time.
const postMessage = async (msg: ToVariantAnalysisMessage) => { | ||
await act(async () => { | ||
// window.postMessage doesn't set the origin correctly, see | ||
// https://github.com/jsdom/jsdom/issues/2745 | ||
window.dispatchEvent( | ||
new MessageEvent("message", { | ||
source: window, | ||
origin: window.location.origin, | ||
data: msg, | ||
}), | ||
); | ||
|
||
// The event is dispatched asynchronously, so we need to wait for it to be handled. | ||
await new Promise((resolve) => setTimeout(resolve, 0)); | ||
}); | ||
}; |
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.
Now that we're using this in at least two tests, do you think it would make sense to move this to a separate file and export the function?
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.
Good idea! I wasn't sure where best to put the shared file. Currently in src/view/common/post-message.ts
but I'm open to better suggestions 😅
Thank you for the helpful comments, @koesie10 😍 |
When pairing on this on Friday, we decided to add the sort/filter information to the Over the weekend I've thought more and I think it's still unclear what the best approach is, but I'm leaning more in favour of having a new message type and just sending two separate messages one after the other. That way the naming of the messages is more clear, and we don't have the sort/filter state being optional in the message. What do you think? These details of the webview messages are purely internal and there's no backwards compatibility concerns, so we could also do this as a followup PR after this one. |
Thanks, @robertbrignull! That seems like a reasonable suggestion. Doesn't sounds too big, so I'll change that in this 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.
LGTM from my point of view. @koesie10 do you want to take another look / have any comments or are you happy?
Thanks for the reviews! I'll merge/fix conflicts once the 1.8.4 release is complete 🚀 |
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.
LGTM, I just found one minor inconsistency
Co-authored-by: Koen Vlaswinkel <[email protected]>
Woops! Thanks, @koesie10. Fixed. |
Slight niggle (just discussed with @robertbrignull offline) is that I haven't yet implemented showing "in progress" repos. If we only display Instead of fixing that in this PR, I'm going to change the default back to Sorry for the noise! 🙉 |
Made @robertbrignull, would you mind giving this another look to make sure it matches what we discussed? 🙇🏽♀️ |
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.
Yep, this matches what I thought we discussed.
I also tried running it and it works well. I'd be happy to ship this.
Adds new settings for configuring the default display of variant analysis results:
This also changes the default display to sort by number of results (instead of by name).
Checklist
ready-for-doc-review
label there.