Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[App Search] Added Sample Response section to Result Settings #95971

Merged
merged 13 commits into from
Apr 5, 2021
Merged

[App Search] Added Sample Response section to Result Settings #95971

merged 13 commits into from
Apr 5, 2021

Conversation

JasonStoltz
Copy link
Member

@JasonStoltz JasonStoltz commented Mar 31, 2021 •
edited
Loading

Summary

This PR adds the Sample Response preview section to the Result Settings page.

latest

Testing:

  • When you load this page, a sample response should always be displayed because an initial query is performed on the initial page render.
  • Subsequently changing the query text in "Sample Response", or any of the "raw" or "snippet" fields will perform a new query and update the response being displayed in Sample Response.
  • Behind the scenes, a query is being performed and the first result of the response is being displayed. So you should expect the result being displayed to change as you update the query text.
  • If you manually enter an invalid number for any of the "size" fields, the validation response will be shown in place of the response.
    • Note: This could be updated to be something that is more i18n friendly, but I was debating whether or not we should.
  • If you enter a query that results in 0 results, a "No results." message will be shown in place of the response.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@JasonStoltz JasonStoltz force-pushed the sample-response branch 2 times, most recently from 7c53f7f to 73532cf Compare April 1, 2021 14:26
@JasonStoltz JasonStoltz marked this pull request as ready for review April 1, 2021 16:31
@JasonStoltz JasonStoltz requested a review from a team April 1, 2021 16:31
@JasonStoltz JasonStoltz added v7.13.0 release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed labels Apr 1, 2021
@byronhulcher byronhulcher self-requested a review April 2, 2021 14:29
byronhulcher
byronhulcher approved these changes Apr 2, 2021
View reviewed changes
Copy link
Contributor

@byronhulcher byronhulcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but I think it could be worth discussing the error/empty messaging for the response with design/prod. I have my own opinions (left in a comment) but don't feel strongly, I think this approach improves some things, and could be updated in a future design pass.

.../applications/app_search/components/result_settings/sample_response/sample_response_logic.ts Show resolved Hide resolved
.../applications/app_search/components/result_settings/sample_response/sample_response_logic.ts
Comment on lines +86 to +89
// 4XX Validation errors are expected, as a user could enter something like 2 as a size, which is out of valid range.
// In this case, we simply render the message from the server as the response.
//
// 5xx Server errors are unexpected, and need to be reported in a flash message.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for this comment

.../applications/app_search/components/result_settings/sample_response/sample_response_logic.ts Outdated Show resolved Hide resolved
x-pack/plugins/enterprise_search/server/routes/app_search/sample_response_search.test.ts Outdated Show resolved Hide resolved
cee-chen
cee-chen reviewed Apr 2, 2021
View reviewed changes
...ications/app_search/components/result_settings/sample_response/sample_response_logic.test.ts
);
});

it('does nothing if an empty object is passed for the resultFields parameter', async () => {
Copy link
Member

@cee-chen cee-chen Apr 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my edification, what's the real world scenario where this would be the case? When an engine has no documents?

I tried testing the current view on an empty engine and the entire UI looked a bit odd. I'm kinda wondering if there are no fields, if we should just return early on the entire view with a "You have no documents/fields/schema" type empty prompt and not bother showing the sample response.

EDIT: I want to add this is not a change request, as I don't mind overly this extra catch for safety, but I do think we could address this scenario better in the UI in a redesign

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, frankly. This happened when I ran the code, so I put a check in for it. I didn't dig deeper. I will though.

There is an additional empty state I haven't implemented yet. I'll do this in a subsequent PR:
empty

Copy link
Member

@cee-chen cee-chen Apr 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right yeah sorry for being unclear - I'm suggesting changing the Kibana UI from the screenshotted standalone UI to hide the sample response column entirely if the engine has no schema/fields (i.e., an early return EuiEmptyPrompt). That way we don't run into the need for this check (although to clarify I'm also fine having it just in case).

Edit for even more clarification: I'm also super fine with suggesting this change in the upcoming work where you implement said empty state, and leaving this alone for now!

@cee-chen
Copy link
Member

cee-chen commented Apr 2, 2021

This overall looks good and QA's great! Most of my comments are fairly minor in the grand scheme of things, feel free to shout at me if it's unclear if something is a change request or not

JasonStoltz and others added 8 commits April 5, 2021 10:45
@JasonStoltz
Merge branch 'master' into sample-response
3170c0e
@JasonStoltz
Move search route
4a6ebdd
@JasonStoltz
Update x-pack/plugins/enterprise_search/public/applications/app_searc…
a0b16c7 
…h/components/result_settings/result_settings_table/non_text_fields_body.tsx

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@JasonStoltz
Update x-pack/plugins/enterprise_search/public/applications/app_searc…
1859f53 
…h/components/result_settings/sample_response/sample_response.tsx

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@JasonStoltz
Update x-pack/plugins/enterprise_search/public/applications/app_searc…
af2ecbf 
…h/components/result_settings/sample_response/sample_response.tsx

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@JasonStoltz
Remove null checks
fe5c3ba
@JasonStoltz
Better VO checkboxes
894fd56
@JasonStoltz
Show response instead of message
08737c4
@cee-chen
Copy link
Member

cee-chen commented Apr 5, 2021

Changes look super great - just 3 quick aria label spelling nits. If no objection I'll batch commit them on your behalf since they're so minor and approve

cee-chen
cee-chen approved these changes Apr 5, 2021
View reviewed changes
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 This looks great - thanks Jason!

@JasonStoltz JasonStoltz enabled auto-merge (squash) April 5, 2021 16:22
@JasonStoltz
Copy link
Member Author

Thanks for the fixes, I'm just a really big fan of the font.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1312 1315 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
enterpriseSearch 1.9MB 1.9MB +4.5KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@JasonStoltz JasonStoltz merged commit ad5f83a into elastic:master Apr 5, 2021
@kibanamachine kibanamachine mentioned this pull request Apr 5, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 5, 2021
@JasonStoltz @kibanamachine
[App Search] Added Sample Response section to Result Settings (elasti…
ec2ae6a 
…c#95971)
@kibanamachine
Copy link
Contributor

💚 Backport successful

✅ 7.x / #96234

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 5, 2021
@kibanamachine @JasonStoltz
[App Search] Added Sample Response section to Result Settings (#95971) (
6776f38 
#96234)

Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 6, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into sr/fix-wizard…
97ee8a6 
…-nav

* 'master' of github.com:elastic/kibana: (106 commits)
  [Lens] don't use eui variables for zindex (elastic#96117)
  Remove /src/legacy (elastic#95510)
  skip flaky suite (elastic#95899)
  [Dashboard] Fix Lens and TSVB chart tooltip positioning relative to global headers (elastic#94247)
  fixes a skipped management x-pack test (elastic#96178)
  [App Search] API logs: Add log detail flyout (elastic#96162)
  [tech-debt] Remove defunct opacity parameters from EUI shadow functions (elastic#96191)
  Add Input Controls project configuration (elastic#96238)
  [file upload] document file upload privileges and provide actionable UI when failures occur (elastic#95883)
  Revert "TS Incremental build exclude test files (elastic#95610)" (elastic#96223)
  [App Search] Added Sample Response section to Result Settings (elastic#95971)
  [Maps] Safe-erase text-field (elastic#94873)
  [RAC][Alert Triage][TGrid] Update the Alerts Table (TGrid) API to implement `renderCellValue` (elastic#96098)
  [Maps] Enable all zoom levels for all users (elastic#96093)
  Use plugin version in its publicPath (elastic#95945)
  [Enterprise Search] Expose core.chrome.setIsVisible for use in Workplace Search (elastic#95984)
  [Workplace Search] Add sub nav and fix rendering bugs in Personal dashboard (elastic#96100)
  [OBS]home page is showing incorrect value of APM throughput (tpm) (elastic#95991)
  [Observability] Exploratory View initial skeleton (elastic#94426)
  [KQL] Fixed styles of KQL textarea for the K8 theme (elastic#96190)
  ...

# Conflicts:
#	x-pack/plugins/snapshot_restore/__jest__/client_integration/helpers/restore_snapshot.helpers.ts
@JasonStoltz JasonStoltz deleted the sample-response branch April 6, 2021 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byronhulcher byronhulcher byronhulcher approved these changes

@cee-chen cee-chen cee-chen approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JasonStoltz @cee-chen @kibanamachine @byronhulcher