-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Search Embeddable] Add highlighting when searching #93178
Conversation
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.
Code LGTM 👍 Since the test coverage of the saved_search embeddable is low, could you add a functional for it to catch it in the future? Theoretically it should work like this test in the the Discover test suite:
kibana/test/functional/apps/discover/_field_data.ts
Lines 44 to 51 in 4614202
it('the search term should be highlighted in the field data', async function () { | |
// marks is the style that highlights the text in yellow | |
const marks = await PageObjects.discover.getMarks(); | |
expect(marks.length).to.be(50); | |
expect(marks.indexOf('php')).to.be(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.
Code only review: Functional tests LGTM!
this.filtersSearchSource!.setField('highlightAll', true); | ||
} else { | ||
this.filtersSearchSource!.removeField('highlightAll'); | ||
} |
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.
It looks like you've made the highlighting conditional, but the Discover app does it unconditionally. Why the difference?
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 don't think we need it once the filter/query is removed, feels cleaner this way
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* [Search Embeddable] Add highlighting when searching * Adding a functional test
* [Search Embeddable] Add highlighting when searching * Adding a functional test
* master: (45 commits) Add outcome of node scripts/build_api_docs (elastic#93399) [Lens] fix long field name on field stats panel doesn't wrap (elastic#93279) [Bug] Fix filter creation for numeric scripted fields in Discover (elastic#93224) [uptime] Fix anomaly alert edit (elastic#93025) Consolidate @babel/* packages and use latest compatible version (elastic#93264) [Search Embeddable] Add highlighting when searching (elastic#93178) [APM] Add missing bottom border to header (elastic#93179) [CI] No longer collect APM span stack traces (elastic#93263) [XY Chart] Fix "No data to display" error when using IP range aggregation to split series (elastic#93024) update generated public api docs API DOCS Step 3/3 (elastic#92929) chore(NA): look for bazel packages on npm_module folder during distributable build (elastic#93262) rename advanced setting ml:fileDataVisualizerMaxFileSize to fileUpload:maxFileSize and increase max geojson upload size to 1GB (elastic#92620) [kbn/optimizer] allow customizing the limits path from the script (elastic#93153) [Alerting][Docs] Adding template for documenting alert and action types (elastic#92830) [jenkins] convert baseline capture job to use tasks (elastic#93288) removing the linked issue in comments from PR (elastic#93303) chore(NA): do not include fs within a storybook build (elastic#93294) [Maps] Update Map extent queries to use bounding box logic for both point and shape queries (elastic#93156) Add searchDuration to EQL and Threshold rules (elastic#93149) ...
Summary
Fixes: #93093
Adds highlighting for filtering and search results in the search embeddable.
filtering_highlight.mp4
Checklist
Delete any items that are not applicable to this PR.
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)- [ ] This was checked for cross-browser compatibilityFor maintainers