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

[Dashboard] EUI Visual Refresh Integration #204442

Merged

cqliu1 merged 8 commits into elastic:main from cqliu1:borealis/dashboard/replace-euiThemeVars-usage

Dec 18, 2024

Contributor

cqliu1 commented Dec 16, 2024 •

edited

Loading

Summary

Related to #203132.

Part of #204590.

This replaces all references to euiThemeVars in favor of the useEuiTheme hook in the dashboard plugin. This also updates the color of the presentational panel border in edit mode from mediumShade, which was deprecated, to BorderBaseFormsControl as the darker border color.

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

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

See some risk examples
...

cqliu1 force-pushed the borealis/dashboard/replace-euiThemeVars-usage branch from a63e9e5 to fd9b122 Compare

December 16, 2024 16:49

cqliu1 added Feature:Dashboard EUI Visual Refresh loe:medium labels

cqliu1 force-pushed the borealis/dashboard/replace-euiThemeVars-usage branch from bb1f68c to bddbc7d Compare

December 16, 2024 17:05


          Replace euiThemeVars usage in presentation panel

6d69138

cqliu1 force-pushed the borealis/dashboard/replace-euiThemeVars-usage branch from bddbc7d to 6d69138 Compare

December 16, 2024 17:08

cqliu1 mentioned this pull request

[Presentation] Replace euiThemeVars with euiTheme #203306

Closed

9 tasks

mgadewoll reviewed

View reviewed changes

.../presentation_panel/public/panel_component/panel_header/presentation_panel_hover_actions.tsx Outdated Show resolved Hide resolved

.../presentation_panel/public/panel_component/panel_header/presentation_panel_hover_actions.tsx Outdated

@@ @@ -142,11 +147,11 @@ export const PresentationPanelHoverActions = ({ @@
                   const hoverActionsWidth =
                     (rightHoverActionsRef.current?.offsetWidth ?? 0) +
                     (dragHandleRef.current?.offsetWidth ?? 0) +
-                    parseInt(euiThemeVars.euiSize, 10) * 2;
+                    parseInt(euiTheme.size.m, 10) * 2;

Contributor

mgadewoll Dec 17, 2024

This should probably be euiTheme.size.base as euiSize was 16px and euiTheme.size.m is 12px
(same for the following occurrences)

📖 size docs, migration overview

.../presentation_panel/public/panel_component/panel_header/presentation_panel_hover_actions.tsx Outdated Show resolved Hide resolved

.../presentation_panel/public/panel_component/panel_header/presentation_panel_hover_actions.tsx Outdated

                           position: absolute;
                           top: 0;
                           display: flex;
                           justify-content: space-between;
-                          padding: 0 ${euiThemeVars.euiSize};
+                          padding: 0 ${euiTheme.size.medium};

Contributor

mgadewoll Dec 17, 2024

Blocking euiTheme.size.medium does not exists. Based on the original value, this likely should be euiTheme.size.base.


          Fix eui sizes

8b6bd86

cqliu1 changed the title ~~[Dashboard][Borealis] Replace eui theme vars usage~~ [Dashboard] EUI Visual Refresh Integration

cqliu1 added Team:Presentation impact:critical v9.0.0 labels

cqliu1 marked this pull request as ready for review

December 17, 2024 18:53

cqliu1 requested a review from a team as a code owner

December 17, 2024 18:53

Contributor

elasticmachine commented Dec 17, 2024

Pinging @elastic/kibana-presentation (Team:Presentation)

cqliu1 added backport:version release_note:skip labels


          [CI] Auto-commit changed files from 'node scripts/notice'

c39729e

nreese reviewed

View reviewed changes

.../presentation_panel/public/panel_component/panel_header/presentation_panel_hover_actions.tsx


		const { euiTheme } = useEuiTheme();

		const EDIT_MODE_OUTLINE = `${euiTheme.border.width.thin} dashed ${euiTheme.colors.borderBaseFormsControl}`;

Contributor

nreese Dec 17, 2024

Can you use useMemo to avoid recreating these string on each render.

Contributor

ThomThomson Dec 17, 2024

I think useMemo usage is best left to things which are expensive to compute. useMemo itself can introduce overhead .

cqliu1 added 4 commits

December 18, 2024 00:12


          Memoize presentation panel border style strings

db7251a


          Merge branch 'borealis/dashboard/replace-euiThemeVars-usage' of https…

54351f5

…://github.com/cqliu1/kibana into borealis/dashboard/replace-euiThemeVars-usage


          Add missing dependency

aa956f5


          undo memoize

404d556

andreadelrio requested review from nreese and mgadewoll

December 18, 2024 15:06

mgadewoll approved these changes

View reviewed changes

Contributor

mgadewoll left a comment

✅ Changes LGTM from EUI side


          Merge branch 'main' into borealis/dashboard/replace-euiThemeVars-usage

561487c

Contributor

elasticmachine commented Dec 18, 2024

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 561487c

Failed CI Steps

Jest Tests #12

Test Failures

[job] [logs] Jest Tests #12 / UserCommentPropertyActions renders the correct number of actions

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
`presentationPanel`	15.8KB	15.6KB	-132.0B

Page load bundle

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

id	before	after	diff
`presentationPanel`	44.0KB	43.9KB	-33.0B

History

💔 Build #261548 failed c39729e

nreese approved these changes

View reviewed changes

cqliu1 merged commit 4a6e1e5 into elastic:main

8 checks passed

cqliu1 mentioned this pull request

[Dashboard] EUI Visual Refresh Integration and QA #204590

Open

10 tasks

JoseLuisGJ pushed a commit to JoseLuisGJ/kibana that referenced this pull request


          [Dashboard] EUI Visual Refresh Integration (elastic#204442)

5aa2fbf

## Summary

Related to elastic#203132.
Closes elastic#204590.

This replaces all references to euiThemeVars in favor of the useEuiTheme
hook in the dashboard plugin. This also updates

### 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](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [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
- [ ] 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)
- [ ] 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](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) 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](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

---------

Co-authored-by: kibanamachine <[email protected]>

kibanamachine added the backport missing label

Contributor

kibanamachine commented Dec 20, 2024

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 204442 locally

3 similar comments

Contributor

kibanamachine commented Dec 23, 2024

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 204442 locally

Contributor

kibanamachine commented Dec 24, 2024

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 204442 locally

Contributor

kibanamachine commented Dec 25, 2024

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 204442 locally

cqliu1 added backport:skip and removed backport missing labels

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

Labels

backport:skip backport:version EUI Visual Refresh Feature:Dashboard impact:critical loe:medium release_note:skip Team:Presentation v9.0.0