-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Kibana 7.0.0 URL field formatter doesn't render relative hyperlinks properly #53265
Conversation
abda56f
to
3b67821
Compare
3b67821
to
21d5d31
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
94fc7df
to
8504b28
Compare
8504b28
to
1ed5526
Compare
Pinging @elastic/kibana-app-arch (Team:AppArch) |
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 LGTM! I think I prefer this approach above the others we have discussed so far.
The only thing I think we should add are some functional tests.
src/plugins/data/public/field_formats_provider/field_formats.ts
Outdated
Show resolved
Hide resolved
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.
Looks real efficiently!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
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 all LGTM, thanks for all of the iterations on this! I think we are in a good place.
I'll approve on my end now as I am going to be unavailable next week, but let's wait to merge until we add a functional test to this PR and an approval from @dmlemeshko
We should also open a PR with the "smaller" fix we discussed (the one-line change in pipeline_helpers/utilities
where we use getConverterFor
instead of convert
), which can merge directly into 7.5
branch, so this can ship in 7.5.2
. Ideally the functional test would be included in that PR as well as this one.
As for the functional test, the main use case I think we should capture is something like this:
- Update an IP field in the index patterns so be of type URL
- Navigate to Discover, assert that the correct HTML is rendered for the IP field
- Navigate to Visualize, create table vis
- Split rows with terms agg on IP field
- Assert that correct HTML is rendered for the IP field
I don't think testing a dashboard is necessary as it should be using the same code which is used in visualize for rendering, but open to your suggestions on this.
…roperly (elastic#53265) * Kibana 7.0.0 URL field formatter doesn't render relative hyperlinks properly * fix CI * fix PR comment / add tests
Closes: #35235
Summary
HtmlContextTypeConvert
,TextContextTypeConvert
types were refactored and unified. Now both converters accept 2 arguments: value and optionsOptions for
HtmlContextTypeConvert
:Options for
TextContextTypeConvert
:parsedUrl
was moved into meta params. Duplicate code was removed across all KibanaChecklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers