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.

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

[Shared UX] Add <NotFound /> prompt #145598

Merged
merged 28 commits into from
Dec 1, 2022
Merged

Conversation

afgomez
Copy link
Contributor

@afgomez afgomez commented Nov 17, 2022

Summary

Creates a shared <NotFound /> prompt to be used when any given consumer needs to show a 404 error.

Screenshot 2022-11-17 at 18 06 12

@afgomez afgomez added release_note:skip Skip the PR/issue when compiling release notes v8.6.0 labels Nov 17, 2022
@afgomez afgomez requested a review from a team November 17, 2022 17:09
loadImage();
}, [colorMode]);

const icon = errorImage ? <EuiImage src={errorImage} alt="" /> : null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The alt attribute is empty on purpose since the image is decoration, not content.

@kellyemurphy
Copy link
Contributor

Hi @afgomez we did this 404 work in the Cloud repo, so you should be able to copy that text.

@afgomez
Copy link
Contributor Author

afgomez commented Nov 18, 2022

Thanks @kellyemurphy! I copied it from the EUI guidelines page. It would make sense to update the text there as well to prevent future confusion.

I'll open a ticket later in their repo.

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Where is this design coming from? Has anyone from design approved this?

packages/shared-ux/prompt/errors/src/not_found.tsx Outdated Show resolved Hide resolved
packages/shared-ux/prompt/errors/src/not_found.tsx Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
@afgomez
Copy link
Contributor Author

afgomez commented Nov 22, 2022

Hi @majagrubic!

Where is this design coming from?

This comes from EUI. It has been implemented in cloud and I have a use case for it in kibana.

Has anyone from design approved this?

I assume so since there are already implementations (unless kibana and cloud follow different design guidelines).

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

I am ok with shipping this once the codeowners file is updated. I will leave it without explicit approval in case @clintandrewhall still has something to add.

packages/shared-ux/prompt/errors/src/not_found.stories.tsx Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
@smith
Copy link
Contributor

smith commented Nov 22, 2022

@majagrubic

Where is this design coming from? Has anyone from design approved this?

After initially speaking with the Shared UX team I spoke with @ryankeairns in our internal #platform-design channel on 2022-09-08 and he confirmed that we should follow the same design as we're using on cloud.

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

This is an awesome contribution, thank you!

A few requests:

  1. consider shared-ux/prompt/not_found as the path.
  2. consider NotFoundPrompt as the component name.

packages/shared-ux/prompt/errors/index.ts Outdated Show resolved Hide resolved
packages/shared-ux/prompt/errors/src/not_found.stories.tsx Outdated Show resolved Hide resolved
packages/shared-ux/prompt/errors/src/not_found.tsx Outdated Show resolved Hide resolved
packages/shared-ux/prompt/errors/src/not_found.tsx Outdated Show resolved Hide resolved
packages/shared-ux/prompt/errors/src/not_found.tsx Outdated Show resolved Hide resolved
packages/shared-ux/prompt/errors/src/not_found.tsx Outdated Show resolved Hide resolved
packages/shared-ux/prompt/errors/src/not_found.tsx Outdated Show resolved Hide resolved
packages/shared-ux/prompt/errors/src/not_found.tsx Outdated Show resolved Hide resolved
packages/shared-ux/prompt/errors/src/not_found.tsx Outdated Show resolved Hide resolved
packages/shared-ux/prompt/errors/src/not_found.tsx Outdated Show resolved Hide resolved
@afgomez
Copy link
Contributor Author

afgomez commented Nov 29, 2022

@clintandrewhall I have addressed all feedback. I have also renamed the plugin name to reflect the new folder name.

Let me know if something else is needed

@afgomez afgomez enabled auto-merge (squash) November 29, 2022 13:14
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/shared-ux-prompt-not-found - 1 +1
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-prompt-not-found - 2 +2

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 109 115 +6
securitySolution 442 448 +6
total +20

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 519 525 +6
total +21

History

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

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

Great work. Thanks for your efforts (and patience) on this.

@afgomez afgomez merged commit df41bfa into elastic:main Dec 1, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.6 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.6:
- [Fleet] Fix agent status computation to support agent v2 uppercase (#146757)

Manual backport

To create the backport manually run:

node scripts/backport --pr 145598

Questions ?

Please refer to the Backport tool documentation

@afgomez afgomez removed the v8.6.0 label Dec 1, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 1, 2022
This was referenced Dec 1, 2022
afgomez pushed a commit that referenced this pull request Dec 6, 2022
## Summary

Closes #144366.

This PR needs to wait for #145598
to use a shared `<NotFound />` component instead of creating its own.

### How to test
- Visit a monitor page with a bogus ID, like
`http://<kibana-base-path>/app/synthetics/monitor/i-dont-exist`

Before:

<img width="1276" alt="Screenshot 2022-11-17 at 15 07 51"
src="https://user-images.githubusercontent.com/57448/202469421-3d065d43-740d-4878-9a64-504c711ddcf9.png">


After:

<img width="1279" alt="Screenshot 2022-11-17 at 15 06 38"
src="https://user-images.githubusercontent.com/57448/202469433-6a6d22e3-8b1a-48f1-9934-f5094b89bac1.png">

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants