Conversation
This commit extends the search progress popover and adds the search jobs section. In the React version the UI for the popover differs slightly when search jobs are enabled vs not enabled. I decided to ignore the differences and only impolemented the style used for the 'search jobs enabled' version, which makes the popover a bit more compact. The logic for validating and creating a search job is encapsulated in a class which makes it easy to conditionally show the search jobs UI. Additional changes: - Skipped items is now a list and uses `<details>` elements which is semantically more correct. We loose the styling of the chevron but I think that's OK. - LoadingSpinner was updated to work inline. - Logging was fixed (?) in the React version to send the correct meta-data. - Added integration tests for the search job popover behavior
| telemetryService.log('SearchJobsSearchFormShown', { validState }, { validState }) | ||
| telemetryRecorder.recordEvent('search.exhaustiveJobs', 'view', { | ||
| metadata: { validState: validState ? 1 : 0 }, | ||
| metadata: { validState: validState === 'valid' ? 1 : 0 }, |
There was a problem hiding this comment.
This seemed like a bug to me since validState is always a non-empty string.
| sentryDNS: undefined, | ||
| }, | ||
| // Playwright specific overwrites | ||
| window.playwrightContext |
There was a problem hiding this comment.
Turns out we have to merge playwright specific overwrites last because when running tests against a local, proxied instance, the injected window.context will overwrite the playwright specific settings.
| offset, | ||
| shift: { padding: 4 }, | ||
| flip: { | ||
| flip: flip ? { |
There was a problem hiding this comment.
I didn't like how the popover would switch from the bottom to the left side when expanding a section would grow the popover beyond the viewport.
In other words: The click target shouldn't move unexpectedly.
| // Must be a root query. We resolve the query here to make sure we | ||
| // resolve operation-specific overrides correctly. | ||
| if (info.parentType.name === 'Query') { | ||
| if (info.parentType.name === 'Query' || info.parentType.name === 'Mutation') { |
There was a problem hiding this comment.
This was necessary to make mutation overwrites work in playwright tests.
stefanhengl
left a comment
stefanhengl
left a comment
There was a problem hiding this comment.
Thanks for getting this in so quickly!! 🙇
One minor concern is the way we handle errors. Felix and I chatted about this on Slack a bit. In the current version we surface the [GraphQL] prefix to the user, which is an artifact of the client we use and the fact that ValidateSearchJob returns a "raw" GraphQL error. Either we decide to handle this in general or we have to update the API in the next release. I am fine with either, but it would be nice to define a general approach we want to follow, not just for Search Jobs.
fkling
deleted the
fkling-srch-838-unable-to-create-search-jobs-from-the-svelte-prototype-need
branch
2 participants
Closes srch-838
This commit extends the search progress popover and adds the search jobs section.
In the React version the UI for the popover differs slightly when search jobs are enabled vs not enabled.
I decided to ignore the differences and only impolemented the style used for the 'search jobs enabled' version, which makes the popover a bit more compact.
The logic for validating and creating a search job is encapsulated in a class which makes it easy to conditionally show the search jobs UI.
Additional changes:
<details>elements which is semantically more correct. We loose the styling of the chevron but I think that's OK.Test plan
Integration tests and manual testing.