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

[Stack Monitoring] Visual refresh changes #204258

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

[Stack Monitoring] Visual refresh changes #204258

wants to merge 26 commits into from
+1,166 −968

Conversation

weronikaolejniczak
Copy link

@weronikaolejniczak weronikaolejniczak commented Dec 13, 2024 •
edited
Loading

Summary

This PR introduces changes to x-pack/plugins/monitoring necessary for the Visual Refresh project (#199715):

  • replacing euiThemeVars with euiTheme context
  • replacing old tokens with euiTheme
  • making sure all color palette functions are run within the context of the EuiProvider

Additionally:

  • I migrated Sass to @emotion/react
  • I migrated euiStyled to @emotion/react
  • I extended emotion.d.ts in tsconfig.json for typing of the EUI theme

closes #8228

QA

We need to test the critical paths in the Stack monitoring, paying close attention to:

  • color palette, visibility and contrast ratio of elements in Amsterdam / Borealis

Specific paths:

  • Monitoring time-series "Zoom out" button hover behavior - x-pack/plugins/monitoring/public/components/chart/monitoring_timeseries_container.tsx
  • Shard allocation (especially color mapping with shard type and status):
    • x-pack/plugins/monitoring/public/components/elasticsearch/shard_allocation/components/assigned.js
    • x-pack/plugins/monitoring/public/components/elasticsearch/shard_allocation/components/shard.js
  • Kuery bar suggestions and autocomplete field:
    • x-pack/plugins/monitoring/public/components/kuery_bar/autocomplete_field.tsx
    • x-pack/plugins/monitoring/public/components/kuery_bar/suggestion_item.tsx

Checklist

  • Unit or functional tests were updated
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

@weronikaolejniczak weronikaolejniczak added EUI Visual Refresh backport:skip This commit does not require backporting labels Dec 13, 2024
@weronikaolejniczak weronikaolejniczak marked this pull request as ready for review December 16, 2024 12:08
@weronikaolejniczak weronikaolejniczak requested review from a team as code owners December 16, 2024 12:08
@weronikaolejniczak weronikaolejniczak added the release_note:skip Skip the PR/issue when compiling release notes label Dec 16, 2024
mgadewoll
mgadewoll requested changes Dec 16, 2024
View reviewed changes
x-pack/plugins/monitoring/public/components/kuery_bar/suggestion_item.tsx Outdated Show resolved Hide resolved
x-pack/plugins/monitoring/public/components/apm/instances/instances.tsx Outdated Show resolved Hide resolved
x-pack/plugins/monitoring/public/components/chart/horizontal_legend.tsx Outdated Show resolved Hide resolved
x-pack/plugins/monitoring/public/components/chart/horizontal_legend.tsx Outdated Show resolved Hide resolved
x-pack/plugins/monitoring/public/components/cluster/listing/listing.js Outdated Show resolved Hide resolved
x-pack/plugins/monitoring/public/components/kibana/cluster_status/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/monitoring/public/components/logstash/pipeline_viewer/views/metric.tsx Outdated Show resolved Hide resolved
x-pack/plugins/monitoring/public/components/logstash/pipeline_viewer/views/statement.tsx Outdated Show resolved Hide resolved
x-pack/plugins/monitoring/public/components/logstash/pipeline_viewer/views/statement.tsx Outdated Show resolved Hide resolved
x-pack/plugins/monitoring/public/components/sparkline/index.js Outdated Show resolved Hide resolved
@andreadelrio andreadelrio changed the title Feat/monitoring visual refresh [Stack Monitoring] Visual refresh changes Dec 16, 2024
@consulthys
Copy link
Contributor

consulthys commented Dec 17, 2024 •
edited
Loading

Thank you everyone for helping us on this, truly appreciated!

I've made a first test run with Amsterdam to verify that everything was still looking "as usual". I've been through all screens and I have nothing to report, everything looks great!

I'm not able to start Kibana with the Borealis theme (errors out), I've shared some details here so I'll do another test run with Borealis once we get that sorted out.

@weronikaolejniczak
Copy link
Author

/ci

@patpscal
Copy link

All LGTM -- I just noticed a badge (Replica) that I'm not sure should be success as a positive reinforcement , or secondary accent - leaving it here!
Screenshot 2024-12-17 at 11 50 32

@consulthys
Copy link
Contributor

All LGTM -- I just noticed a badge (Replica) that I'm not sure should be success as a positive reinforcement , or secondary accent - leaving it here!

Totally agree with @patpscal it can definitely be secondary accent

@weronikaolejniczak
Copy link
Author

@patpscal @consulthys great catch! Just fyi, accentSecondary is removed for the EuiBadge component. We can pass in the color prop anything that we want, it doesn't have to be danger | warning | accent | default | primary | success.

Screenshot 2024-12-17 at 13 36 50

@consulthys
Copy link
Contributor

@patpscal @consulthys great catch! Just fyi, accentSecondary is removed for the EuiBadge component. We can pass in the color prop anything that we want, it doesn't have to be danger | warning | accent | default | primary | success.

Thanks @weronikaolejniczak I think instead of success we just need a green-ish color

@weronikaolejniczak
Copy link
Author

weronikaolejniczak commented Dec 17, 2024 •
edited
Loading

@andreadelrio @mgadewoll @patpscal @consulthys thank you so much for the testing and suggestions! 💚 I updated the PR with the changes.

Let me know the final decision on those legend badge colors and I'd appreciate a double-check of the paths I defined in the PR description (where I made riskier style changes).

Testing:

- ✅ Removing wrapping <div> (addressing: #204258 (comment))
File: x-pack/plugins/monitoring/public/components/beats/listing/listing.js
Path: Menu bar > Under "Management" > Stack Monitoring > Under "Beats" > Beats (localhost url)

Amsterdam Borealis
Screenshot 2024-12-17 at 14 18 30

- ✅ Removing wrapping <div> (addressing: #204258 (comment))
File: x-pack/plugins/monitoring/public/components/kibana/instances/instances.tsx
Path: Menu bar > Under "Management" > Stack Monitoring > Under "Kibana" > Instances (localhost url)

Amsterdam Borealis
Screenshot 2024-12-17 at 14 16 45

Warning

The issue with the Badge overlapping the next column is reproducible regardless of the changes made in this PR. If you have a ready solution, I'd appreciate it! Otherwise, let's leave it like so.

Untested paths (fyi @consulthys):

- Removing wrapping <div> (addressing: #204258 (comment))
File: x-pack/plugins/monitoring/public/components/logstash/listing/listing.js
Path: Menu bar > Under "Management" > Stack Monitoring > Under "Logstash" > Instances

- Removing wrapping <div> (addressing: #204258 (comment))
File: x-pack/plugins/monitoring/public/components/apm/instances/instances.tsx
Path: Menu bar > Under "Management" > Stack Monitoring > Under "APM server" > APM servers (localhost url)

- Removing !important flag (addressing: #204258 (comment))
File: x-pack/plugins/monitoring/public/components/logstash/pipeline_viewer/views/metric.tsx
Path: Menu bar > Under "Management" > Stack Monitoring > Under "Logstash" > Pipeline viewer

@andreadelrio
Copy link
Contributor

andreadelrio commented Dec 17, 2024 •
edited
Loading

@patpscal @consulthys great catch! Just fyi, accentSecondary is removed for the EuiBadge component. We can pass in the color prop anything that we want, it doesn't have to be danger | warning | accent | default | primary | success.

Thanks @weronikaolejniczak I think instead of success we just need a green-ish color

I think sticking with success is our best option here. No harm in using it.

consulthys
consulthys approved these changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@consulthys consulthys left a comment

Choose a reason for hiding this comment

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

Everything looks good on both Amsterdam and Borealis.
Thank you everyone again for helping us on this!

mgadewoll
mgadewoll approved these changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ Thanks for the additional changes! Great work on this 🎉
I just left two small non-blocking comments.

x-pack/plugins/monitoring/public/components/kuery_bar/suggestion_item.tsx Outdated Show resolved Hide resolved
x-pack/plugins/monitoring/public/components/elasticsearch/nodes/nodes.js Outdated Show resolved Hide resolved
@weronikaolejniczak
Copy link
Author

/ci

@weronikaolejniczak weronikaolejniczak requested a review from a team as a code owner December 26, 2024 11:41
weronikaolejniczak and others added 24 commits December 26, 2024 13:03
@weronikaolejniczak
refactor(monitoring): replace euiStyled with css from @emotion/react
188a75e
@weronikaolejniczak
chore(monitoring): extend emotion.d.ts in tsconfig.json
1ed7345
@weronikaolejniczak
fix(monitoring): use euiTheme instead of theme.eui
f86f46c
@weronikaolejniczak
feat(monitoring): replace euiThemeVars with euiTheme context
15e2eb2 
- Replace euiZLevel1 with levels.maskBelowHeader
@weronikaolejniczak
refactor(monitoring): migrate Sass to @emotion/react
75ecf79 
- Remove tintOrShade() because of minimal difference of 2%
- Use EuiProvider context for the theme
@weronikaolejniczak
chore(monitoring): remove redundant comment
9fe776e
@weronikaolejniczak
fix(monitoring): add missing theme variables
caa4759
@weronikaolejniczak
chore(monitoring): update test snapshots
ae0ad27
@weronikaolejniczak
refactor(monitoring): simplify passing boolean prop
89a7460
@weronikaolejniczak
chore(monitoring): disable jsx-a11y eslint error
3910c8b
@weronikaolejniczak
fix(monitoring): address feedback from PR
604193c 
- use font-weight tokens
- use `euiFontSize` for the font-size
- use `euiTextTruncate` utility
- use semantic tokens e.g. `backgroundBasePlain`
- use `logicalCSS` with border-left in shard allocation
- fix width calculation in shard allocation
- rename `css` style variables
@kibanamachine @weronikaolejniczak
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
6e00934 
…-fix'
@weronikaolejniczak
chore(monitoring): revert removing JSDoc comments
3599076
@weronikaolejniczak
fix(monitoring): address PR feedback
c03c5ea 
- use `logicalCSS` utility for `padding/margin/border-top/bottom/left/right`
- destructure `euiTheme` wherever applicable
@weronikaolejniczak
fix(monitoring): import logicalCSS function
be1259e
@weronikaolejniczak
fix(monitoring): remove div wrapping setup mode badge
537c102
@weronikaolejniczak
chore: remove @elastic/observability-design from x-pack/plugins/monit…
4d9caf0 
…oring
@weronikaolejniczak
refactor(monitoring): destructure euiTheme
ac73699
@weronikaolejniczak
fix(monitoring: fix euiFontSize usages
2b5a670
@weronikaolejniczak
fix(monitoring): remove important flag
bdf91ff
@weronikaolejniczak
chore(monitoring): update test snapshots
2625905
@weronikaolejniczak
fix(monitoring): add data-test-subj props to tables
7da0729
@weronikaolejniczak
chore(monitoring): update test snapshots
d9fd8c3
@weronikaolejniczak
refactor(monitoring): remove redundant divs
a64a966
@weronikaolejniczak weronikaolejniczak force-pushed the feat/monitoring-visual-refresh branch from 2e2db71 to a64a966 Compare December 26, 2024 12:09
@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 26, 2024 •
edited
Loading

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #49 / Monitoring app Cluster listing with trial license clusters cluster table and toolbar filters for non-existent cluster
  • [job] [logs] FTR Configs #49 / Monitoring app Cluster listing with trial license clusters cluster table and toolbar filters for non-existent cluster

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
monitoring 501 419 -82

Async chunks

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

id before after diff
monitoring 538.9KB 521.6KB -17.4KB

Page load bundle

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

id before after diff
monitoring 25.7KB 25.5KB -217.0B
Unknown metric groups

async chunk count

id before after diff
monitoring 6 7 +1

ESLint disabled line counts

id before after diff
monitoring 25 26 +1

Total ESLint disabled count

id before after diff
monitoring 32 33 +1

History

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgadewoll mgadewoll mgadewoll approved these changes

@consulthys consulthys consulthys approved these changes

@patpscal patpscal patpscal approved these changes

@akowalska622 akowalska622 Awaiting requested review from akowalska622

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting EUI Visual Refresh release_note:skip Skip the PR/issue when compiling release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Visual Refresh changes in Stack monitoring
7 participants
@weronikaolejniczak @consulthys @patpscal @andreadelrio @elasticmachine @mgadewoll @kibanamachine