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.

Sign up for GitHub

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

Jump to bottom

[Logs Explorer] Add "Add data" button #166266

Merged
merged 11 commits into from
Sep 14, 2023
Merged

[Logs Explorer] Add "Add data" button #166266

merged 11 commits into from
Sep 14, 2023

Conversation

tonyghiani
Copy link
Contributor

📓 Summary

Closes #165486

This work restructured the entry point for the 2 different header menus (stateful, serverless) and added both of them the Add data button to navigate to the onboarding page.

Stateful

Screenshot 2023-09-12 at 16 01 16

Serverless

Screenshot 2023-09-12 at 15 49 09

@tonyghiani tonyghiani added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes labels Sep 12, 2023
@tonyghiani tonyghiani requested review from a team as code owners September 12, 2023 14:08
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

yngrdyn
yngrdyn approved these changes Sep 12, 2023
View reviewed changes
Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

APM Changes LGTM

Marco Antonio Ghiani added 2 commits September 12, 2023 17:44
@weltenwort weltenwort self-requested a review September 12, 2023 16:15
weltenwort
weltenwort reviewed Sep 13, 2023
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Navigating directly by clicking it works 👍 The completely changed order of elements when comparing stateful and serverless felt surprising, but I assume this is current design?

x-pack/plugins/observability_log_explorer/public/components/log_explorer_top_nav_menu.tsx Outdated

return (
<EuiButton
onClick={navigateToOnboarding}
Copy link
Member

Choose a reason for hiding this comment

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

Can we enhance this opening in a new tab also works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen.Recording.2023-09-14.at.09.29.22.mov

@tonyghiani
Copy link
Contributor Author

Navigating directly by clicking it works 👍 The completely changed order of elements when comparing stateful and serverless felt surprising, but I assume this is current design?

Correct, here you can find the different design for the two modes.

weltenwort
weltenwort reviewed Sep 14, 2023
View reviewed changes
x-pack/plugins/observability_log_explorer/public/components/log_explorer_top_nav_menu.tsx Outdated
return (
<EuiButton
href={onboardingUrl}
target="_blank"
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this always open in a new tab now? Sorry for being unclear, but this would be the behavior we're aiming for with such buttons in Kibana when possible:

  • clicking the button navigates to the on-boarding without a full page reload
  • selecting "open in a new tab/window" on the button opens the on-boarding in a new tab/window

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see what you mean!
I just added a change to fix this link behavior and the discover one, thanks for clarifying this!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observabilityLogExplorer 36 41 +5

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
observabilityOnboarding 15 16 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observabilityLogExplorer 11.5KB 13.9KB +2.4KB
Unknown metric groups

API count

id before after diff
observabilityOnboarding 15 17 +2

History

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

weltenwort
weltenwort approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

x-pack/plugins/observability_log_explorer/public/components/log_explorer_top_nav_menu.tsx
services.discover.locator?.navigate(discoverLinkParams);
};

const discoverLinkProps = getRouterLinkProps({
Copy link
Member

Choose a reason for hiding this comment

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

This works 👍 As we amass more cross-app links over time we might want to switch to

kibana/src/plugins/kibana_react/public/app_links/redirect_app_link.tsx

Line 41 in 5106852

export const RedirectAppLinks: FunctionComponent<RedirectCrossAppLinksProps> = ({
at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know about its existence, thanks for the heads-up!

@tonyghiani tonyghiani merged commit 39cf371 into elastic:main Sep 14, 2023
@kibanamachine kibanamachine added v8.11.0 backport:skip This commit does not require backporting labels Sep 14, 2023
@achyutjhunjhunwala achyutjhunjhunwala changed the title [Log Explorer] Add "Add data" button [Logs Explorer] Add "Add data" button Jan 3, 2024
@tonyghiani tonyghiani deleted the 165486-add-data-applicationa-button branch January 24, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yngrdyn yngrdyn yngrdyn approved these changes

@weltenwort weltenwort weltenwort approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Explorer] Add "add data" application navigation button
7 participants
@tonyghiani @elasticmachine @apmmachine @kibana-ci @weltenwort @yngrdyn @kibanamachine