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

Migrate Session View to Borealis #207710

Merged
merged 7 commits into from
Jan 23, 2025
Merged

Conversation

maxcold
Copy link
Contributor

@maxcold maxcold commented Jan 22, 2025

Summary

Migrating Session View to Borealis:

  • adjust button colors to the new color palette after feedback from @codearos
  • get rid of the use of euiVars in favor of color tokens from euiTheme
  • replace custom color buttonsBackgroundNormalDefaultPrimary with eui token
  • get rid of EuiTextColor wrapping for Details panel values, after feedback from @codearos , the different color usage there has unclear purpose and doesn't work well with the new theme

Follow up after:

Overall migration guide from EUI:

Amsterdam vs Borealis comparison screenshots for CSP and Session View:
https://www.figma.com/design/XPKYLgZcoI61V3RUfHoHg9/Untitled?node-id=41-42&t=zLvulagbqCiXhrAo-0

How to test

Upload data

node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/process_events --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn
node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/alerts --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn 
node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/io_events --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn

navigate to Alerts and choose the last 3 years in the date picker. There should be alerts with the Session View button available

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • 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

@maxcold maxcold added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v9.0.0 Team:Cloud Security Cloud Security team related labels Jan 22, 2025
@maxcold
Copy link
Contributor Author

maxcold commented Jan 22, 2025

/ci

@maxcold maxcold marked this pull request as ready for review January 23, 2025 09:56
@maxcold maxcold requested a review from a team as a code owner January 23, 2025 09:56
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security)

{exec}
</EuiTextColor>
{eventAction && (
<EuiTextColor color="subdued" css={styles.executableAction}>
<EuiTextColor color="default" css={styles.executableAction}>
Copy link
Contributor

@alexreal1314 alexreal1314 Jan 23, 2025

Choose a reason for hiding this comment

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

@maxcold general question - why do we have EuiTextColor and EuiText? looks like EuiText supports everything EuiTextColor does and more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexreal1314 I don't know tbh, a good question for EUI team. Seems like a convenience sugar according to the docs https://eui.elastic.co/#/display/text#coloring-text EuiTextColor is meant to color parts of the text and translate into span, but the same can be achieved by passing component and color props to EuiText as it seems

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #20 / useGetCaseConnectors shows a toast error message when an error occurs in the response

Metrics [docs]

Async chunks

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

id before after diff
sessionView 355.0KB 354.9KB -72.0B

Page load bundle

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

id before after diff
sessionView 48.7KB 45.4KB -3.2KB

History

@maxcold maxcold merged commit 3ca23fa into main Jan 23, 2025
8 checks passed
@maxcold maxcold deleted the csp-borealis-update-session-view branch January 23, 2025 12:31
viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
## Summary

Migrating Session View to Borealis:
- adjust button colors to the new color palette after feedback from
@codearos
- get rid of the use of `euiVars` in favor of color tokens from
`euiTheme`
- replace custom color `buttonsBackgroundNormalDefaultPrimary` with eui
token
- get rid of `EuiTextColor` wrapping for Details panel values, after
feedback from @codearos , the different color usage there has unclear
purpose and doesn't work well with the new theme

Follow up after:
- elastic#205136

Overall migration guide from EUI:
- elastic#199715

Amsterdam vs Borealis comparison screenshots for CSP and Session View:

https://www.figma.com/design/XPKYLgZcoI61V3RUfHoHg9/Untitled?node-id=41-42&t=zLvulagbqCiXhrAo-0

### How to test

Upload data
```
node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/process_events --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn
node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/alerts --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn 
node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/io_events --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn
```

navigate to Alerts and choose the last 3 years in the date picker. There
should be alerts with the Session View button available

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [x] 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.
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Copy link
Contributor

@opauloh opauloh left a comment

Choose a reason for hiding this comment

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

LGTM - tested locally

qn895 pushed a commit to qn895/kibana that referenced this pull request Jan 23, 2025
## Summary

Migrating Session View to Borealis:
- adjust button colors to the new color palette after feedback from
@codearos
- get rid of the use of `euiVars` in favor of color tokens from
`euiTheme`
- replace custom color `buttonsBackgroundNormalDefaultPrimary` with eui
token
- get rid of `EuiTextColor` wrapping for Details panel values, after
feedback from @codearos , the different color usage there has unclear
purpose and doesn't work well with the new theme

Follow up after:
- elastic#205136

Overall migration guide from EUI:
- elastic#199715

Amsterdam vs Borealis comparison screenshots for CSP and Session View:

https://www.figma.com/design/XPKYLgZcoI61V3RUfHoHg9/Untitled?node-id=41-42&t=zLvulagbqCiXhrAo-0

### How to test

Upload data
```
node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/process_events --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn
node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/alerts --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn 
node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/io_events --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn
```

navigate to Alerts and choose the last 3 years in the date picker. There
should be alerts with the Session View button available

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [x] 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.
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
JoseLuisGJ pushed a commit to JoseLuisGJ/kibana that referenced this pull request Jan 27, 2025
## Summary

Migrating Session View to Borealis:
- adjust button colors to the new color palette after feedback from
@codearos
- get rid of the use of `euiVars` in favor of color tokens from
`euiTheme`
- replace custom color `buttonsBackgroundNormalDefaultPrimary` with eui
token
- get rid of `EuiTextColor` wrapping for Details panel values, after
feedback from @codearos , the different color usage there has unclear
purpose and doesn't work well with the new theme

Follow up after:
- elastic#205136

Overall migration guide from EUI:
- elastic#199715

Amsterdam vs Borealis comparison screenshots for CSP and Session View:

https://www.figma.com/design/XPKYLgZcoI61V3RUfHoHg9/Untitled?node-id=41-42&t=zLvulagbqCiXhrAo-0

### How to test

Upload data
```
node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/process_events --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn
node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/alerts --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn 
node scripts/es_archiver load x-pack/test/functional/es_archives/session_view/io_events --es-url http://elastic:changeme@localhost:9200 --kibana-url http://elastic:changeme@localhost:5601/kbn
```

navigate to Alerts and choose the last 3 years in the date picker. There
should be alerts with the Session View button available

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [x] 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.
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
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 Team:Cloud Security Cloud Security team related v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants