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

Fixing styles for `euiKeyPadMenu` #8294

Open

ek-so wants to merge 6 commits into elastic:eui-theme/borealis from ek-so:key-pad-menu-styles

Contributor

ek-so commented Jan 27, 2025 •

edited

Loading

Summary

This PR fixes an issue mentioned in issue #158. Based on the discussion there and also on Borealis new color token rules, the changes include:

Remapping of the colors that are supported both in Amsterdam and Borealis;
Conditionally adding different interactive states to Borealis only (Amsterdam remains the same).

Changes made for Borealis include:

Removed jumping animation on hover
Added background on hover in addition to underline
Used a hovered background for the item on click, when it's not selected yet
Removed shadow entirely

Showcasing:

Amsterdam

Borealis

QA:

Browser QA
- Checked in both light and dark modes
- Checked in mobile
- Checked in Chrome, Safari, Edge, and Firefox
- Checked for accessibility including keyboard-only and screenreader modes
Docs site QA
- Added documentation
- Props have proper autodocs (using @default if default values are missing) and playground toggles
- Checked Code Sandbox works for any docs examples
Code quality checklist
- Added or updated jest and cypress tests
- Updated visual regression tests
Release checklist
- A changelog entry exists and is marked appropriately.
- If applicable, added the breaking change issue label (and filled out the breaking change checklist)
Designer checklist
- If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

ek-so added 5 commits

January 26, 2025 10:01


          Style changes

f7a4139


          Simplify code for focused state (selected item) in Borealis

351b99a


          Prettier

382d895


          Update key_pad_menu_item.styles.ts

900dc3a


          Update key_pad_menu_item.styles.ts

eb1837f

ek-so changed the base branch from main to eui-theme/borealis

January 27, 2025 09:18


          Changelog

ed3123e

kibanamachine commented Jan 27, 2025

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_8294/
Storybook: https://eui.elastic.co/pr_8294/storybook
New docs: https://eui.elastic.co/pr_8294/new-docs

Collaborator

elasticmachine commented Jan 27, 2025

💚 Build Succeeded

Buildkite Build
Commit: ed3123e

ek-so marked this pull request as ready for review

January 27, 2025 09:48

ek-so requested a review from a team as a code owner

January 27, 2025 09:48

tkajtoch self-requested a review

January 29, 2025 13:13

mgadewoll reviewed

View reviewed changes

packages/eui/src/components/key_pad_menu/key_pad_menu_item.styles.ts

-                    selected: css`
-                      background-color: ${euiTheme.components
-                        .keyPadMenuItemBackgroundDisabledSelect};
+                    selected: hasVisColorAdjustment

Contributor

mgadewoll Feb 5, 2025

Let's update this to ensure that all styles are part of the Emotion css, prevent the lint error and we can also simplify as it's the same style rule:

selected: css`
  background-color: ${hasVisColorAdjustment
    ? euiTheme.components.keyPadMenuItemBackgroundDisabledSelect
    : euiTheme.colors.backgroundBaseDisabled};
`

packages/eui/src/components/key_pad_menu/key_pad_menu_item.styles.ts

                     ${euiCanAnimate} {
                       transition: background-color ${euiTheme.animation.fast} ease-in,
                         box-shadow ${euiTheme.animation.fast} ease-in;
                     }
                   `,
                   enabled: css`
+                    /* stylelint-disable no-extra-semicolons */

Contributor

mgadewoll Feb 5, 2025

We can remove this comment by rewriting the styles a bit. Generally assuring the style rule is set and only values are condition seems to work better in those cases.
I'll add suggestions per line.

packages/eui/src/components/key_pad_menu/key_pad_menu_item.styles.ts

-                          transform: translateY(0);
-                        }
-                      }
+                      ${hasVisColorAdjustment

Contributor

mgadewoll Feb 5, 2025

Here we can prevent the Emotion-related semicolon lint error by separating multiline styles into a standalone variable.

const focusTransformStyles = `
    ${euiShadow(euiThemeContext, 's')};

    ${euiCanAnimate} {
      .euiKeyPadMenuItem__icon {
        transform: translateY(0);
      }
    }
  `;
  
// usage
 ${hasVisColorAdjustment
  ? focusTransformStyles
  : `background-color: ${euiTheme.colors.backgroundBaseInteractiveHover}`}

packages/eui/src/components/key_pad_menu/key_pad_menu_item.styles.ts

		box-shadow: none;

		${hasVisColorAdjustment

Contributor

mgadewoll Feb 5, 2025

We can prevent the lint error by using it this way:

background-color: ${
   hasVisColorAdjustment
     ? euiTheme.focus.backgroundColor
     : euiTheme.colors.backgroundBaseInteractiveHover
};

packages/eui/src/components/key_pad_menu/key_pad_menu_item.styles.ts

-                    background-color: ${euiTheme.focus.backgroundColor};
+                    color: ${euiTheme.colors.textHeading};
+                    ${hasVisColorAdjustment

Contributor

mgadewoll Feb 5, 2025

Let's update to this to prevent the lint error:

background-color: ${hasVisColorAdjustment
        ? euiTheme.focus.backgroundColor
        : ''};

packages/eui/src/components/key_pad_menu/key_pad_menu_item.styles.ts

-                      color: ${euiTheme.colors.primaryText};
+                      color: ${euiTheme.colors.textPrimary};
+                      ${hasVisColorAdjustment

Contributor

mgadewoll Feb 5, 2025

Let's update to this to prevent the lint error:

background-color: ${!hasVisColorAdjustment
          ? euiTheme.colors.backgroundBaseInteractiveSelect
          : ''};

packages/eui/src/components/key_pad_menu/key_pad_menu_item.styles.ts

                       .euiKeyPadMenuItem__icon {
-                        filter: grayscale(100%);
+                        ${hasVisColorAdjustment ? `filter: grayscale(100%);` : ``}

Contributor

mgadewoll Feb 5, 2025

We can update to:

filter: ${hasVisColorAdjustment ? 'grayscale(100%)' : ''};

packages/eui/changelogs/upcoming/8294.md

		@@ -0,0 +1,6 @@
		- Updates styles for keypad menu in Borealis

Contributor

mgadewoll Feb 5, 2025

We don't add changelogs to changes on eui-theme/borealis as it's a feature branch.
You can add the 'skip-chagelog` label.

We'll add a changelog when merging Borealis to main for any relevant changes.

packages/eui/changelogs/upcoming/8294.md


		Accessibility

		- Made keypad menu items in Dark mode visible on hover

Contributor

mgadewoll Feb 5, 2025

Was this actually done? I don't see any dark mode specific changes 🤔

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

Labels

None yet