Skip to content

[Discover] Remove inline top nav#208297

Merged
davismcphee merged 2 commits intoelastic:mainfrom
davismcphee:discover-remove-top-nav-inline
Jan 27, 2025
Merged

[Discover] Remove inline top nav#208297
davismcphee merged 2 commits intoelastic:mainfrom
davismcphee:discover-remove-top-nav-inline

Conversation

@davismcphee
Copy link
Copy Markdown
Contributor

@davismcphee davismcphee commented Jan 25, 2025 •
edited
Loading

Summary

This is followup work to #203685 that removes DiscoverTopNavInline now that Logs Explorer is gone, since the component is no longer used. I also noticed that top nav style overrides we use weren't applied to the inline top nav, so this fixes that as well.

Checklist

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

@davismcphee davismcphee added Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// backport:prev-minor labels Jan 25, 2025
@davismcphee davismcphee self-assigned this Jan 25, 2025
@davismcphee davismcphee added backport:skip This PR does not require backporting and removed backport:prev-minor labels Jan 27, 2025
@davismcphee davismcphee marked this pull request as ready for review January 27, 2025 03:14
@davismcphee davismcphee requested review from a team as code owners January 27, 2025 03:14
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jan 27, 2025

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jan 27, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 986 985 -1

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
discover 164 160 -4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 830.4KB 828.0KB -2.4KB

Page load bundle

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

id before after diff
discover 45.2KB 44.6KB -617.0B
securitySolutionServerless 26.6KB 26.5KB -30.0B
serverlessSearch 22.9KB 22.9KB -30.0B
total -677.0B
Unknown metric groups

API count

id before after diff
discover 211 207 -4

cc @davismcphee

jughosta
jughosta approved these changes Jan 27, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

LGTM 👍

src/platform/plugins/shared/discover/public/customizations/types.ts
@@ -26,17 +26,4 @@ export interface DiscoverCustomizationContext {
* Display mode in which discover is running
*/
displayMode: DiscoverDisplayMode;
Copy link
Copy Markdown
Contributor

@jughosta jughosta Jan 27, 2025

Choose a reason for hiding this comment

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

Where do we use the remaining displayMode?

Copy link
Copy Markdown
Contributor Author

@davismcphee davismcphee Jan 27, 2025

Choose a reason for hiding this comment

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

It's passed by Timeline to DiscoverContainer when embedding ES|QL in Timeline, mainly to avoid impacting global state like breadcrumbs, etc.

logeekal
logeekal approved these changes Jan 27, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@logeekal logeekal left a comment

Choose a reason for hiding this comment

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

Code review looks good 👍

andreadelrio
andreadelrio approved these changes Jan 27, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Design changes LGTM

@davismcphee davismcphee merged commit dcdffff into elastic:main Jan 27, 2025
@davismcphee davismcphee deleted the discover-remove-top-nav-inline branch January 27, 2025 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jughosta jughosta jughosta approved these changes

@TattdCodeMonkey TattdCodeMonkey TattdCodeMonkey approved these changes

@andreadelrio andreadelrio andreadelrio approved these changes

@logeekal logeekal logeekal approved these changes

Assignees

@davismcphee davismcphee

Labels

backport:skip This PR does not require backporting Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// v9.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@davismcphee @elasticmachine @jughosta @TattdCodeMonkey @andreadelrio @logeekal @kibanamachine