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

[Obs AI Assistant] Borealis theme integration #200814

Merged
merged 9 commits into from
Nov 27, 2024

Conversation

viduni94
Copy link
Contributor

@viduni94 viduni94 commented Nov 19, 2024

Summary

Integrates changes from the Borealis theme to the components owned by obs-ai-assistant, obs-knowledge teams and for files related to kbn-ai-assistant.

Checklist

  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

@viduni94 viduni94 added release_note:enhancement backport:skip This commit does not require backporting Team:Obs AI Assistant Observability AI Assistant EUI Visual Refresh labels Nov 19, 2024
@viduni94 viduni94 self-assigned this Nov 19, 2024
@viduni94 viduni94 requested a review from a team November 19, 2024 20:25
@viduni94
Copy link
Contributor Author

viduni94 commented Nov 19, 2024

Hi @elastic/eui-team
I have a question w.r.t replacing the colors used for darkShade and darkestShade.
I want to update the below references to these colors since they are marked as deprecated.

However, I'm not sure about the approach I should follow for this.
For example, the hex color for darkestShade in the light theme is #2B394F which maps to blueGrey120 --> shade120 (in semantic colors)
However, I can't find a token in the light theme semantic colors for shade120 to replace darkestShade.

Would you be able to clarify how I should replace shade related variables?

@viduni94 viduni94 force-pushed the borealis-theme-updates branch from b9a7eb7 to 02fe1ad Compare November 20, 2024 12:48
@mgadewoll
Copy link
Contributor

Hi @elastic/eui-team I have a question w.r.t replacing the colors used for darkShade and darkestShade. I want to update the below references to these colors since they are marked as deprecated.

However, I'm not sure about the approach I should follow for this. For example, the hex color for darkestShade in the light theme is #2B394F which maps to blueGrey120 --> shade120 (in semantic colors) However, I can't find a token in the light theme semantic colors for shade120 to replace darkestShade.

Would you be able to clarify how I should replace shade related variables?

@viduni94 Thanks for asking about it!
These legacy shade tokens (lightestShade, darkestShade etc) are mark as deprecated (they are not used in the new theme) but not required to be removed just yet. They have been mapped for now on EUI side. There will be a separate update round for those tokens once there is a clear way forward on how to move off them.

That's why they were not mentioned directly as changes in this document and we noted:

A practical example of this can be seen by looking at our shade colors. The naming of these tokens is based on their appearance -- dark. light, darkest, etc. Over time, we will deprecate these tokens in favor of tokens that are based on usage -- "border", "background", etc. This will be a key concept that allows us to iterate and evolve the Kibana UI over time -- cleanly and easily.

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.

Changes LGTM from EUI side 🦄

@viduni94 viduni94 force-pushed the borealis-theme-updates branch from 7beecbf to 2714969 Compare November 20, 2024 16:34
@viduni94 viduni94 marked this pull request as ready for review November 20, 2024 17:01
@viduni94 viduni94 requested review from a team as code owners November 20, 2024 17:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ai-assistant (Team:Obs AI Assistant)

@viduni94 viduni94 force-pushed the borealis-theme-updates branch from 2714969 to c8e5f24 Compare November 20, 2024 17:01
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Nov 20, 2024
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 docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@viduni94
Copy link
Contributor Author

The changes from my end are complete for all files owned by obs-ai-assistant and obs-knowledge.

However, I noticed one issue with the Obs AI Assistant UI which is related to files in x-pack/packages/kbn-ai-assistant
This is owned by search-kibana team, but thought of making a note here for visibility as we use the components in kbn-ai-assistant.

In Amsterdam, the comment list items have a darker border. The Borealis theme shows it correctly.

Amsterdam:
Image

Borealis:
Image

This issue was mentioned by another team in the eui Slack channel (thread)
I'll check on this again, once the fix is merged.

@mgadewoll
Copy link
Contributor

mgadewoll commented Nov 20, 2024

The changes from my end are complete for all files owned by obs-ai-assistant and obs-knowledge.

However, I noticed one issue with the Obs AI Assistant UI which is related to files in x-pack/packages/kbn-ai-assistant This is owned by search-kibana team, but thought of making a note here for visibility as we use the components in kbn-ai-assistant.

In Amsterdam, the comment list items have a darker border. The Borealis theme shows it correctly.

Amsterdam: Image

Borealis: Image

This issue was mentioned by another team in the eui Slack channel (thread) I'll check on this again, once the fix is merged.

@viduni94 We're aware of the issue and already have a fix on EUI side for it.
It will be added on Kibana side with the next Borealis release.

@viduni94
Copy link
Contributor Author

@viduni94 We're aware of the issue and already have a fix on EUI side for it. It will be added on Kibana side with the next Borealis release.

Thank you @mgadewoll

@viduni94 viduni94 force-pushed the borealis-theme-updates branch from c8e5f24 to 756a735 Compare November 21, 2024 12:26
@viduni94
Copy link
Contributor Author

/oblt-deploy

@viduni94 viduni94 force-pushed the borealis-theme-updates branch from 756a735 to 1fc2487 Compare November 21, 2024 16:08
@viduni94 viduni94 requested a review from a team as a code owner November 21, 2024 18:03
Copy link

@patpscal patpscal left a comment

Choose a reason for hiding this comment

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

Hi, @viduni94! Overall, it looks pretty good to me, but I noticed:

AI logo icon

image

To ensure a correct transition between themes, it's recommended (Slack conversation) to use the euiTheme.colors.title token instead of fill="#343741"
Note: The logo might change to be monochromatic in the future, so if updating it is a considerable effort, we can reevaluate.

Chart

image

Backgrounds look a bit off, but that's expected currently, as Charts have not been updated yet. No changes are needed!

@viduni94 viduni94 force-pushed the borealis-theme-updates branch from e64a3a6 to 4b2247a Compare November 26, 2024 14:00
@viduni94
Copy link
Contributor Author

viduni94 commented Nov 26, 2024

Hi, @viduni94! Overall, it looks pretty good to me, but I noticed:

AI logo icon

image To ensure a correct transition between themes, it's recommended ([Slack conversation](https://elastic.slack.com/archives/C060SDYTZ96/p1731421921528569?thread_ts=1731316042.876429&cid=C060SDYTZ96)) to use the `euiTheme.colors.title` [token](https://eui.elastic.co/#/theming/colors/values#text-colors) instead of fill="#343741" Note: The logo might change to be monochromatic in the future, so if updating it is a considerable effort, we can reevaluate.

Hi @patpscal
Thank you for the review.

I tried adding euiTheme.colors.title, however, it's marked as deprecated.
image

Therefore I added euiTheme.colors.textHeading. Would this work?

image
image

https://github.com/elastic/kibana/pull/200814/files#diff-99dc84658998ef72039163efdc16b1b4b3f6bff6ce03a1f329caadc16794958cR42

@viduni94 viduni94 requested a review from patpscal November 26, 2024 14:22
@ek-so
Copy link
Contributor

ek-so commented Nov 26, 2024

@viduni94 we were planning to use euiTheme.colors.textParagraph for the icons, coz they usually stand close to regular text paragraph, so I imagine it would be more consistent if we use it here as well, wdyt @patpscal? And that said, the difference between heading and paragraph is minimal anyway.

Just a note. That is correct, we have euiTheme.colors.textHeading and euiTheme.colors.textParagaraph now as main tokens to work with text.

@patpscal
Copy link

@viduni94 we were planning to use euiTheme.colors.textParagraph for the icons, coz they usually stand close to regular text paragraph, so I imagine it would be more consistent if we use it here as well, wdyt @patpscal? And that said, the difference between heading and paragraph is minimal anyway.

Just a note. That is correct, we have euiTheme.colors.textHeading and euiTheme.colors.textParagaraph now as main tokens to work with text.

Sounds good! Let's use euiTheme.colors.textParagaraph

@viduni94
Copy link
Contributor Author

@viduni94 we were planning to use euiTheme.colors.textParagraph for the icons, coz they usually stand close to regular text paragraph, so I imagine it would be more consistent if we use it here as well, wdyt @patpscal? And that said, the difference between heading and paragraph is minimal anyway.
Just a note. That is correct, we have euiTheme.colors.textHeading and euiTheme.colors.textParagaraph now as main tokens to work with text.

Sounds good! Let's use euiTheme.colors.textParagaraph

I've made the change and updated the deployment.

Copy link

@patpscal patpscal left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🎉

@viduni94 viduni94 force-pushed the borealis-theme-updates branch from 21f081d to 6579011 Compare November 26, 2024 15:57
@viduni94 viduni94 removed request for a team and kkurstak November 26, 2024 16:01
@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 26, 2024

💛 Build succeeded, but was flaky

  • Buildkite Build
  • Commit: 6579011
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-200814-657901108eb2

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #16 / management Index patterns on aliases discover verify hits should be able to discover and verify no of hits for alias2

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
observabilityAIAssistantApp 237.5KB 237.5KB -71.0B
searchAssistant 160.2KB 160.2KB -3.0B
total -74.0B

Page load bundle

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

id before after diff
observabilityAIAssistant 47.0KB 47.0KB -7.0B
observabilityAIAssistantApp 8.7KB 8.6KB -54.0B
searchAssistant 4.7KB 4.7KB -54.0B
total -115.0B

History

cc @viduni94

@viduni94 viduni94 added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Nov 27, 2024
@viduni94 viduni94 merged commit a800360 into elastic:main Nov 27, 2024
28 checks passed
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
## Summary

Integrates changes from the Borealis theme to the components owned by
`obs-ai-assistant`, `obs-knowledge` teams and for files related to
`kbn-ai-assistant`.

### Checklist

- [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)

---------

Co-authored-by: kibanamachine <[email protected]>
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 ci:project-deploy-observability Create an Observability project EUI Visual Refresh release_note:skip Skip the PR/issue when compiling release notes Team:Obs AI Assistant Observability AI Assistant v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants