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

[Fleet][Eui Visual Refresh] Update text color tokens #204537

Conversation

jillguyonnet
Copy link
Contributor

Summary

Closes #201989

This PR updates color variables to the up-to-date naming per #199715 (comment).

Impacted elements:

  1. Read-only icon buttons in Fleet and Integrations top bar.
  2. Danger-styled context menu item for deleting package policy from agent policy.

I have also reviews all occurrences of color="text throughout fleet and found these to be part of the following components:

  • EuiButton
  • EuiButtonEmpty
  • EuiButtonIcon
  • EuiIcon

The use of color="text" seems to be valid for these cases.

Screenshots

Amsterdam

Read-only Fleet top bar:

fleet-readonly-amsterdam

Read-only Integrations top bar:

integrations-readonly-amsterdam

Delete integration context menu item:

delete-integration-amsterdam

Borealis

Read-only Fleet top bar:

fleet-readonly-borealis

Read-only Integrations top bar:

integrations-readonly-borealis

Delete integration context menu item:

delete-integration-borealis

@jillguyonnet jillguyonnet requested a review from a team as a code owner December 17, 2024 10:35
@jillguyonnet jillguyonnet self-assigned this Dec 17, 2024
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Dec 17, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@jillguyonnet jillguyonnet added Team:Fleet Team label for Observability Data Collection Fleet team EUI Visual Refresh backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes and removed Team:Fleet Team label for Observability Data Collection Fleet team labels Dec 17, 2024
@weronikaolejniczak
Copy link
Contributor

@jillguyonnet LGTM 💚

<EuiContextMenuItem
{...props}
css={css`
color: ${theme.euiTheme.colors.textDanger};
Copy link
Contributor

@mgadewoll mgadewoll Dec 17, 2024

Choose a reason for hiding this comment

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

Non blocking cleanup suggestion:
This is perfectly fine, but you can also use the object notation, which removes the need for importing css and using useEuiTheme specifically (since it's only used for this one case).

css={({ euiTheme }) => ({
  color: euiTheme.colors.textDanger,
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @mgadewoll thanks - I had tried that but ran into

TypeError: Cannot read properties of undefined (reading 'colors')

Here's the full component:

import React from 'react';
import type { EuiContextMenuItemProps } from '@elastic/eui';
import { EuiContextMenuItem } from '@elastic/eui';

export const DangerEuiContextMenuItem = (props: EuiContextMenuItemProps) => {
  return (
    <EuiContextMenuItem
      {...props}
      css={({ euiTheme }) => ({
        color: euiTheme.colors.textDanger,
      })}
    />
  );
};

Did I miss anything?

I found a similar occurrence with a note that an error is expected, leading to elastic/eui#8123. This comment suggests that the issue should be resolved though.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jillguyonnet Ah right, then your plugin doesn't have the required type set up yet.
You'll need to add types for your plugin to support this. (emotion docs)

Here an example of the required module.
And this then should be included in your tsconfig.json (example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this! I'm capturing some frontend cleanup for Fleet UI in a dedicated task, I will add this in order to make getting the last EUI visual refresh PR over the line a priority.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, that makes perfect sense. It wasn't blocking for this PR 👍

Copy link
Contributor

@criamico criamico left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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
fleet 1.7MB 1.7MB +122.0B

cc @jillguyonnet

@jillguyonnet jillguyonnet merged commit a1b3b88 into elastic:main Dec 18, 2024
20 checks passed
@jillguyonnet jillguyonnet deleted the fleet/201989-update-text-color-variables branch December 18, 2024 08:07
JoseLuisGJ pushed a commit to JoseLuisGJ/kibana that referenced this pull request Dec 19, 2024
## Summary

Closes elastic#201989

This PR updates color variables to the up-to-date naming per
elastic#199715 (comment).

Impacted elements:
1. Read-only icon buttons in Fleet and Integrations top bar.
2. Danger-styled context menu item for deleting package policy from
agent policy.

I have also reviews [all
occurrences](https://github.com/search?q=repo%3Aelastic%2Fkibana+path%3Ax-pack%2Fplugins%2Ffleet%2Fpublic+color%3D%22text&type=code)
of `color="text` throughout fleet and found these to be part of the
following components:
- `EuiButton`
- `EuiButtonEmpty`
- `EuiButtonIcon`
- `EuiIcon`

The use of `color="text"` seems to be valid for these cases.

## Screenshots

### Amsterdam

Read-only Fleet top bar:

<img width="1918" alt="fleet-readonly-amsterdam"
src="https://github.com/user-attachments/assets/c6c58a75-b79b-45a9-abef-f25014a7c8c6"
/>

Read-only Integrations top bar:

<img width="1918" alt="integrations-readonly-amsterdam"
src="https://github.com/user-attachments/assets/fa2a5b10-4aca-46ee-bb59-c0f62091c322"
/>

Delete integration context menu item:

<img width="1918" alt="delete-integration-amsterdam"
src="https://github.com/user-attachments/assets/c2375413-373b-4cdf-b907-0d5f577e1fbe"
/>

### Borealis

Read-only Fleet top bar:

<img width="1918" alt="fleet-readonly-borealis"
src="https://github.com/user-attachments/assets/aad169c6-52b0-4707-87a5-1e0babd59a43"
/>

Read-only Integrations top bar:

<img width="1918" alt="integrations-readonly-borealis"
src="https://github.com/user-attachments/assets/08d1cec7-285d-47a3-958b-63b5a22d9b19"
/>

Delete integration context menu item:

<img width="1918" alt="delete-integration-borealis"
src="https://github.com/user-attachments/assets/9cf20708-2a3f-4252-98ac-1ebca12bd8e6"
/>
benakansara pushed a commit to benakansara/kibana that referenced this pull request Jan 2, 2025
## Summary

Closes elastic#201989

This PR updates color variables to the up-to-date naming per
elastic#199715 (comment).

Impacted elements:
1. Read-only icon buttons in Fleet and Integrations top bar.
2. Danger-styled context menu item for deleting package policy from
agent policy.

I have also reviews [all
occurrences](https://github.com/search?q=repo%3Aelastic%2Fkibana+path%3Ax-pack%2Fplugins%2Ffleet%2Fpublic+color%3D%22text&type=code)
of `color="text` throughout fleet and found these to be part of the
following components:
- `EuiButton`
- `EuiButtonEmpty`
- `EuiButtonIcon`
- `EuiIcon`

The use of `color="text"` seems to be valid for these cases.

## Screenshots

### Amsterdam

Read-only Fleet top bar:

<img width="1918" alt="fleet-readonly-amsterdam"
src="https://github.com/user-attachments/assets/c6c58a75-b79b-45a9-abef-f25014a7c8c6"
/>

Read-only Integrations top bar:

<img width="1918" alt="integrations-readonly-amsterdam"
src="https://github.com/user-attachments/assets/fa2a5b10-4aca-46ee-bb59-c0f62091c322"
/>

Delete integration context menu item:

<img width="1918" alt="delete-integration-amsterdam"
src="https://github.com/user-attachments/assets/c2375413-373b-4cdf-b907-0d5f577e1fbe"
/>

### Borealis

Read-only Fleet top bar:

<img width="1918" alt="fleet-readonly-borealis"
src="https://github.com/user-attachments/assets/aad169c6-52b0-4707-87a5-1e0babd59a43"
/>

Read-only Integrations top bar:

<img width="1918" alt="integrations-readonly-borealis"
src="https://github.com/user-attachments/assets/08d1cec7-285d-47a3-958b-63b5a22d9b19"
/>

Delete integration context menu item:

<img width="1918" alt="delete-integration-borealis"
src="https://github.com/user-attachments/assets/9cf20708-2a3f-4252-98ac-1ebca12bd8e6"
/>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Jan 13, 2025
## Summary

Closes elastic#201989

This PR updates color variables to the up-to-date naming per
elastic#199715 (comment).

Impacted elements:
1. Read-only icon buttons in Fleet and Integrations top bar.
2. Danger-styled context menu item for deleting package policy from
agent policy.

I have also reviews [all
occurrences](https://github.com/search?q=repo%3Aelastic%2Fkibana+path%3Ax-pack%2Fplugins%2Ffleet%2Fpublic+color%3D%22text&type=code)
of `color="text` throughout fleet and found these to be part of the
following components:
- `EuiButton`
- `EuiButtonEmpty`
- `EuiButtonIcon`
- `EuiIcon`

The use of `color="text"` seems to be valid for these cases.

## Screenshots

### Amsterdam

Read-only Fleet top bar:

<img width="1918" alt="fleet-readonly-amsterdam"
src="https://github.com/user-attachments/assets/c6c58a75-b79b-45a9-abef-f25014a7c8c6"
/>

Read-only Integrations top bar:

<img width="1918" alt="integrations-readonly-amsterdam"
src="https://github.com/user-attachments/assets/fa2a5b10-4aca-46ee-bb59-c0f62091c322"
/>

Delete integration context menu item:

<img width="1918" alt="delete-integration-amsterdam"
src="https://github.com/user-attachments/assets/c2375413-373b-4cdf-b907-0d5f577e1fbe"
/>

### Borealis

Read-only Fleet top bar:

<img width="1918" alt="fleet-readonly-borealis"
src="https://github.com/user-attachments/assets/aad169c6-52b0-4707-87a5-1e0babd59a43"
/>

Read-only Integrations top bar:

<img width="1918" alt="integrations-readonly-borealis"
src="https://github.com/user-attachments/assets/08d1cec7-285d-47a3-958b-63b5a22d9b19"
/>

Delete integration context menu item:

<img width="1918" alt="delete-integration-borealis"
src="https://github.com/user-attachments/assets/9cf20708-2a3f-4252-98ac-1ebca12bd8e6"
/>
viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
## Summary

Closes elastic#201989

This PR updates color variables to the up-to-date naming per
elastic#199715 (comment).

Impacted elements:
1. Read-only icon buttons in Fleet and Integrations top bar.
2. Danger-styled context menu item for deleting package policy from
agent policy.

I have also reviews [all
occurrences](https://github.com/search?q=repo%3Aelastic%2Fkibana+path%3Ax-pack%2Fplugins%2Ffleet%2Fpublic+color%3D%22text&type=code)
of `color="text` throughout fleet and found these to be part of the
following components:
- `EuiButton`
- `EuiButtonEmpty`
- `EuiButtonIcon`
- `EuiIcon`

The use of `color="text"` seems to be valid for these cases.

## Screenshots

### Amsterdam

Read-only Fleet top bar:

<img width="1918" alt="fleet-readonly-amsterdam"
src="https://github.com/user-attachments/assets/c6c58a75-b79b-45a9-abef-f25014a7c8c6"
/>

Read-only Integrations top bar:

<img width="1918" alt="integrations-readonly-amsterdam"
src="https://github.com/user-attachments/assets/fa2a5b10-4aca-46ee-bb59-c0f62091c322"
/>

Delete integration context menu item:

<img width="1918" alt="delete-integration-amsterdam"
src="https://github.com/user-attachments/assets/c2375413-373b-4cdf-b907-0d5f577e1fbe"
/>

### Borealis

Read-only Fleet top bar:

<img width="1918" alt="fleet-readonly-borealis"
src="https://github.com/user-attachments/assets/aad169c6-52b0-4707-87a5-1e0babd59a43"
/>

Read-only Integrations top bar:

<img width="1918" alt="integrations-readonly-borealis"
src="https://github.com/user-attachments/assets/08d1cec7-285d-47a3-958b-63b5a22d9b19"
/>

Delete integration context menu item:

<img width="1918" alt="delete-integration-borealis"
src="https://github.com/user-attachments/assets/9cf20708-2a3f-4252-98ac-1ebca12bd8e6"
/>
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 EUI Visual Refresh release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet][EUI Visual refresh] Rename color variables to use updated names
6 participants