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

kibana-cloud-security-posture owned UX udjustment for borealis #205136

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

maxcold
Copy link
Contributor

@maxcold maxcold commented Dec 24, 2024

Summary

This PR covers the changes required for the new Borealis theme https://github.com/elastic/security-team/issues/11230

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

@elasticmachine
Copy link
Contributor

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!
  • Click to trigger kibana-deploy-project-from-pr for this PR!

import { CSSObject } from '@emotion/react';
import { useEuiTheme } from '../../hooks';

export const useButtonStyles = () => {
const { euiTheme, euiVars } = useEuiTheme();
const { euiTheme } = useEuiTheme();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this file I tried replacing vis colors with color tokens, but it requires alignment with designers as there is no match to the old vis colors

@@ -37,7 +37,7 @@ export const useStyles = (tty?: Teletype, show?: boolean) => {
padding: `${size.m} ${size.base}`,
};

const windowBoundsColor = transparentize(colors.ghost, 0.6);
const windowBoundsColor = transparentize(colors.ghost, 0.6); // TODO: replace transparentize with color token
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how to test tty_player

@@ -11,8 +11,13 @@ import { useEuiTheme } from '../../../hooks';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't how to test tty player

@@ -36,7 +41,7 @@ export const useStyles = (progress: number) => {
if (selected) {
return euiVars.terminalOutputMarkerAccent;
}
return euiVars.euiColorVis1;
return isAmsterdam(themeName) ? euiVars.euiColorVis1 : euiVars.euiColorVis2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -84,16 +89,24 @@ export const useStyles = (progress: number) => {
"input[type='range']::-webkit-slider-thumb": customThumb,
"input[type='range']::-moz-range-thumb": customThumb,
'.euiRangeHighlight__progress': {
backgroundColor: euiVars.euiColorVis0_behindText,
backgroundColor: isAmsterdam(themeName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

_behindText won't be available in Borealis, advice is to use the normal color

@@ -33,10 +33,10 @@ export const useEuiTheme = (...props: EuiThemeProps): EuiThemeReturn => {

const extraEuiVars: ExtraEuiVars = {
// eslint-disable-next-line @typescript-eslint/naming-convention
euiColorVis6_asText: shade(themeVars.euiColorVis6, 0.335),
buttonsBackgroundNormalDefaultPrimary: '#006DE4',
euiColorVis6_asText: shade(themeVars.euiColorVis6, 0.335), // TODO: check in Borealis. Replce with proper color token and remove shade
Copy link
Contributor Author

Choose a reason for hiding this comment

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

euiColorVis6 becomes pink in Borealis and there is no matching color to the Amsterdam color in the new palette. requires discussion with designers

severity: VulnSeverity,
euiTheme: EuiThemeComputed
): string => {
// TOOD: remove old mapping when severity palette is fixed (atm it is inverted)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should be able to just use the new severity color palette for both Amsterdam and Borealis, but currently there is a bug in elastic/eui#8247 that the severity colors for Amsterdam are inverted. Keeping the old mapping until the severity color palette is fixed

return euiThemeName?.toLowerCase().includes('amsterdam');
};

// TODO: replace with severity color palette
Copy link
Contributor Author

Choose a reason for hiding this comment

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

all these todos should go away when we use only the severity palette for both themes


switch (severity) {
case VULNERABILITIES_SEVERITY.LOW:
return euiTheme.colors.vis.euiColorVis0; // TODO: use color from the severity palette?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

severity paletter introduced in elastic/eui#8247 is missing green color which is used for low severity alerts and vulnerabilities. There is a discussion between EUI team and security designers on how to go about it. The workaround is to use euiColorVis0 which is green in both themes. But if security designers agree to switch to blue info color for low severity signales, this line should be changed to euiTheme.colors.vis.euiColorSeverity1

// TODO: replace with severity color palette
// TODO: replace with euiTheme.colors.vis.* from useEuiTheme hook
export const getCvsScoreColor = (score: number, euiTheme: EuiThemeComputed): string | undefined => {
// TOOD: remove old mapping when severity palette is fixed (atm it is inverted)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as for getSeverityStatusColor

@@ -61,7 +60,6 @@ export const ControlGeneralViewResponse = ({
const [isPopoverOpen, setPopoverOpen] = useState(false);
const styles = useStyles();
const selectorStyles = useSelectorStyles();
const visColorsBehindText = euiPaletteColorBlindBehindText();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

behind text colors won't be available in Borealis. Even though cloud_defend is being deprecated, replacing for safety to a similar semantic tokens

version="1.1"
>
<path
fill={euiTheme.colors.fullShade}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

shade colors are recommended to be replaced too, but there is no clear guidance yet on what to replace them with.

const { score, vector, version } = vectorScore;
return (
<>
<EuiFlexGroup
alignItems="center"
css={css`
background: ${euiThemeVars.euiColorLightestShade};
padding: ${euiThemeVars.euiSizeXS} ${euiThemeVars.euiSizeS};
background: ${euiTheme.colors.backgroundLightText};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as shade colors should be replaced and shouldn't be used for background, replacing with more suitable token. But the color is slightly different

@@ -44,7 +44,7 @@ export const useStyles = () => {
borderRadius: euiTheme.border.radius.small,
border: euiTheme.border.thin,
bottom: '-25px',
boxShadow: `0 ${size.xs} ${size.xs} ${transparentize(euiTheme.colors.shadow, 0.04)}`,
boxShadow: `0 ${size.xs} ${size.xs} ${transparentize(euiTheme.colors.shadow, 0.04)}`, // TODO: replace transparentize with color token
Copy link
Contributor Author

Choose a reason for hiding this comment

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

k8s_security should be decommissioned in 9.0 and serverless, therefore not touching, but leaving a comment in case the dashboard will be revived in the future

@@ -30,6 +30,7 @@ export const KUBERNETES_COLLECTION_FIELDS: KubernetesCollectionMap = {
containerImage: CONTAINER_IMAGE_NAME,
};

// TODO: update color scheme for Borealis
Copy link
Contributor Author

Choose a reason for hiding this comment

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

k8s_security should be decommissioned in 9.0 and serverless, therefore not touching, but leaving a comment in case the dashboard will be revived in the future

@@ -68,7 +68,7 @@ const getFindingsStats = (
}
),
count: failedFindingsStats,
color: euiThemeVars.euiColorVis9,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

euiColorVis9 became yellow in Borealis. vis colors shouldn't be used for semantic purposes exactly for this reason - the vis color can change to pretty much anything. Changing to danger, while it is a different color, brighter one, and it will also change for Amsterdam. need confirmation from designers

isInvestigated?: boolean;
}

export const useStyles = ({ minimal = false, isInvestigated = false }: StylesDeps) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these styles were not used. Removing so that we don't need to think about color palette

@@ -15,8 +15,8 @@ export const useStyles = (minimal = false, isInvestigated = false) => {
const cached = useMemo(() => {
const { colors, font, size, border } = euiTheme;

const dangerBorder = transparentize(colors.danger, 0.2);
const dangerBackground = transparentize(colors.danger, 0.08);
const dangerBorder = transparentize(colors.danger, 0.2); // TODO: replace transparentize with color token
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here and in pretty much every place in session_view we need to replace transparentize and other color functions with color tokens. But it requires input from designers, on which tokens to use exactly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants