-
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
[Discover] Fix link from dashboard saved search to Discover #92937
[Discover] Fix link from dashboard saved search to Discover #92937
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
@@ -94,5 +103,25 @@ export default function ({ getService, getPageObjects }) { | |||
await PageObjects.discover.waitForDiscoverAppOnScreen(); | |||
await PageObjects.discover.waitForDocTableLoadingComplete(); | |||
}); | |||
|
|||
it('navigates to context view from embeddable', async () => { |
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.
thanx for adding a functional test!
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.
Thanks!! It also tests the doc view
of Discover which seems to be a blind spot in functional testing of Discover (Might be because it was a separate plugin without coverage, migrated to the Discover folder). I think the title should be navigate to doc view from embeddable
@@ -78,5 +86,26 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { | |||
} | |||
expect(disabledFilterCounter).to.be(TEST_FILTER_COLUMN_NAMES.length); | |||
}); | |||
|
|||
// Skipped because there is a bug in the data grid row expansion | |||
it.skip('navigates to context view from embeddable', async () => { |
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.
👍
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.
should now be safe to unskip (and change to the title to navigate to doc view), since it was resolved in #92999
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.
Tested in Chrome on Mac OS and works as expected. Not sure what the functional test failure is, I haven't found any issues with the context view as such. It there are no actual issues, feel free to merge once that's settled
I figured out the issue in the tests. There were two problems in the functional test as written:
|
I previously commented about the outdated code style of the |
@@ -44,6 +44,7 @@ | |||
index-pattern="indexPattern" | |||
filter="filter" | |||
class="kbnDocTable__row" | |||
data-test-subj="docTableRow{{ row['$$_isAnchor'] ? ' docTableAnchorRow' : ''}}" |
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.
Nit: docTableAnchorRow
should not be necessary here, it's just used in context view and there infinite-scroll
is set to true. It's here added to the paginated doc table that's used in the embeddable.
@@ -12,19 +12,32 @@ const filterManager = ({ | |||
getGlobalFilters: () => [], | |||
getAppFilters: () => [], | |||
} as unknown) as FilterManager; | |||
const addBasePath = (path: string) => `/base/${path}`; |
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.
Nit: /base/${path}
-> /base${path}
and the expected url would be /base/app/....
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.
Thanks a lot for fixing this! 👍 Code LGTM, just a few minor comments, tested locally in Safari. works as expected.
QQ: @flash1293 there were some redirects for |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
…92937) * [Discover] Fix link from dashboard saved search to Discover * Fix tests that weren't fully testing the navigation * Fix snapshot * Fix test navigation to context app by reverting to previous * Unskip functional test and fix issue in data grid * Respond to review comments
…92937) * [Discover] Fix link from dashboard saved search to Discover * Fix tests that weren't fully testing the navigation * Fix snapshot * Fix test navigation to context app by reverting to previous * Unskip functional test and fix issue in data grid * Respond to review comments
…93500) * [Discover] Fix link from dashboard saved search to Discover * Fix tests that weren't fully testing the navigation * Fix snapshot * Fix test navigation to context app by reverting to previous * Unskip functional test and fix issue in data grid * Respond to review comments Co-authored-by: Kibana Machine <[email protected]>
…93499) * [Discover] Fix link from dashboard saved search to Discover * Fix tests that weren't fully testing the navigation * Fix snapshot * Fix test navigation to context app by reverting to previous * Unskip functional test and fix issue in data grid * Respond to review comments Co-authored-by: Kibana Machine <[email protected]>
* master: (48 commits) Fix wrong import in data plugin causing 100kB bundle increase (elastic#93448) [Fleet] Correctly track install status of an integration (elastic#93464) Reviews data frame analytics UI text (elastic#93033) Display multiple copyable fields for process.args in resolver node detail panel (elastic#93280) [Security Solution][Detections] ML Popover overflow fix (elastic#93525) chore(NA): do not use execa on bazel workspace status update script (elastic#93532) Bump dependencies (elastic#93511) [dev/build_ts_refs] support disabling the ts-refs build completely (elastic#93529) [Security Solution] fix data provider cypress test (elastic#93465) Fix service map for All environment single service (elastic#93517) [Fleet] Fix package version comparaison in the UI (elastic#93498) [alerting] adds doc on JSON-expanded action variables and task manager max_workers (elastic#92720) [dev/build_ts_refs] ignore type checking failures when building ts refs (elastic#93473) [core-new-docs] Adds a dev-doc for core documentation (elastic#92976) remove opacity from maps legacy style (elastic#93456) [Security Solution][Lists] Escape quotes in list ids and quote the id in KQL query (elastic#93176) Revert "Make tests deterministic by providing unique timestamps (elastic#93350)" [Discover] Fix link from dashboard saved search to Discover (elastic#92937) update public api docs App Search - Polishing Analytics Views (elastic#92939) ...
Steps to test:
Also, test:
Fixes #92904
Checklist