-
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
Update page for missing Short URL #171679
Conversation
946c016
to
c3175c0
Compare
Thanks for putting this together, @tsullivan. Per your request, I've taken a look at this and the originating issue posted by @vadimkibana. Is there a compelling reason to not simply use the standard Kibana 404 error here? If I'm attempting to navigate to a saved object that no longer exists (via short or long URL), this is what I would expect to happen. |
90fdc2a
to
3195a3b
Compare
6e62725
to
c285b77
Compare
e886297
to
24bdc5d
Compare
|
||
const defaultBody = i18n.translate('share.urlService.redirect.components.Error.body', { | ||
defaultMessage: | ||
'The stored URL for this link could not be opened. This could be due to lost Kibana Saved Object data.', |
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 think the text could be improved.
I like the text from the screenshot better #171679 (comment)
Also the URL might be broken because it just was incorrect (e.g. mistaken when manually typed or not fully copied)
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.
Maybe something like Sorry, the object you're looking for can't be found at this URL. It might have been removed or maybe it never existed.
// Using the current URL containing "/app/r/", make a URL to the root basePath | ||
// by trimming that part to end up at the Home app or project home. | ||
const currentUrl = window.location.href; | ||
const newUrl = currentUrl.replace(/\/app\/r\/.*/, ''); |
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.
Why wouldn't we use home href from settings instead?
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.
++ to use the defaultRoute
from settings, prepended with the app basePath
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 good apart from suggestion to improve copy and if '/' redirects to defaultRoute
from uiSettings
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
ESLint disabled line counts
miscellaneous assets size
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @tsullivan |
This reverts commit e6f3f06.
Hey, sorry this had to be reverted with #172334. The canvas shareable runtime build is skipped on pull requests by default, but had a build failure later. The build can be opted-in with the Logs are available at https://buildkite.com/elastic/kibana-artifacts-snapshot/builds/3610#018c22ec-6c1e-4b63-92de-58d63d688f94 |
## Summary This PR re-opens #171679 which had to be reverted due to CI instability at the time it was originally mergted. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
Closes #169935
Screenshots
Before
After
Testing
Checklist
Delete any items that are not applicable to this PR.