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

[Serverless] Update observability side navigation #160866

Merged
merged 7 commits into from
Jul 3, 2023

Conversation

kpatticha
Copy link
Contributor

@kpatticha kpatticha commented Jun 29, 2023

Update once again the side navigation tree to match the latest mocks - https://www.figma.com/file/S4fn8L4j8fG1H6331Lw3kb/IA%2FNavigation?type=design&node-id=1265-151762&mode=design

Before

image

After

Screen.Recording.2023-06-29.at.12.39.02.mov

Notes for reviewers

@kpatticha kpatticha requested review from a team as code owners June 29, 2023 10:45
@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.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@kpatticha kpatticha added release_note:skip Skip the PR/issue when compiling release notes apm:serverless v8.10.0 labels Jun 29, 2023
@kpatticha kpatticha requested a review from formgeist June 29, 2023 10:55
link: 'fleet',
},
{
id: 'users_and_roles',
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 external links are not defined yet. I added the default https://cloud.elastic.co but we can remove them until we have the links

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with it but maybe it'd be good to have an issue to not forget about this, wdyt?

Copy link
Contributor

@MiriamAparicio MiriamAparicio left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

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

Is there a chance we can change the link color to text for these external links?

CleanShot 2023-06-30 at 10 24 33@2x

Can we remove the collapsible accordion for the Observability menu?

CleanShot 2023-06-30 at 10 24 48@2x

The Observability and gear icon can be reduced to 16px or if you're using the EuiIcon component to render the icons, it's the size m.

Thanks for getting all these changes in 🙏

id: 'settings',
children: [
{
link: 'monitoring',
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not the right spot for a comment, but we can link to the /app/management app view. This will eventually be replaced with a director overview of the management options available for the project type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. Fixed it here

499459d

Screen.Recording.2023-06-30.at.11.12.59.mov

@kpatticha kpatticha requested a review from a team as a code owner June 30, 2023 09:12
@kpatticha
Copy link
Contributor Author

Is there a chance we can change the link color to text for these external links?

CleanShot 2023-06-30 at 10 24 33@2x Can we remove the collapsible accordion for the Observability menu? CleanShot 2023-06-30 at 10 24 48@2x The Observability and gear icon can be reduced to 16px or if you're using the `EuiIcon` component to render the icons, it's the size `m`.

These changes will require changing the shared navigation component. if you don't mind I will do it in a follow up PR to avoid blocking this work

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

As I put in my comment, the "Project settings" should be exposed by the shared ux component as a preset so we can use it in other side nav.

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

Changes in packages/deeplinks/management folder lgtm

@formgeist
Copy link
Contributor

Is there a chance we can change the link color to text for these external links?
CleanShot 2023-06-30 at 10 24 33@2x
Can we remove the collapsible accordion for the Observability menu?
CleanShot 2023-06-30 at 10 24 48@2x
The Observability and gear icon can be reduced to 16px or if you're using the EuiIcon component to render the icons, it's the size m.

These changes will require changing the shared navigation component. if you don't mind I will do it in a follow up PR to avoid blocking this work

All good 👍

@kpatticha
Copy link
Contributor Author

@elasticmachine merge upstream

@kpatticha kpatticha enabled auto-merge (squash) June 30, 2023 14:52
@kpatticha kpatticha requested a review from sebelga June 30, 2023 15:03
@kpatticha
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/deeplinks-management 2 0 -2

Page load bundle

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

id before after diff
serverlessObservability 21.7KB 22.4KB +722.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 14 16 +2
securitySolution 411 415 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 15 17 +2
securitySolution 490 494 +4
total +6

History

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

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks 👍

@kpatticha kpatticha merged commit 831e858 into elastic:main Jul 3, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:serverless backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants