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

<EuiBeacon> would be more useful if it had a color property #6315

Closed
qpointsystems opened this issue Oct 17, 2022 · 7 comments · Fixed by #6420
Closed

<EuiBeacon> would be more useful if it had a color property #6315

qpointsystems opened this issue Oct 17, 2022 · 7 comments · Fixed by #6420

Comments

@qpointsystems
Copy link

In the EUI documentation, the EuiBeacon component is defined to be used as "...to draw visual attention to a specific location or element."

The current component displays with a slowly blinking icon in the green color.

This component would likely be more useful if it had a color property that could default to 'green' for backwards compatibility but also allow the user of the component to set a different color (e.g., 'warning', etc) in different UI contexts.

@qpointsystems
Copy link
Author

Fully acknowledging that a developer can use css (e.g., style={{ backgroundColor: 'red' }} to accomplish the same effect. But for consistency across the EUI framework, a formal 'color' property would IMO fit better.

@dawitamene
Copy link
Contributor

@miukimiu Hi! I would like to work on this one, could you please assign this to me?

@miukimiu
Copy link
Contributor

miukimiu commented Nov 3, 2022

Thanks, @dawitam11. Let me know if you need help implementing it.

@dawitamene
Copy link
Contributor

@miukimiu Thank you!

export const COLORS = [
  'primary',
  'subdued',
  'success',
  'accent',
  'danger',
  'warning',
] as const;

export type BeaconColor = typeof COLORS[number];

export type EuiBeaconProps = Omit<HTMLAttributes<HTMLDivElement>, 'children'> &
  CommonProps & {
    /**
     * Height and width of the center circle. Value is passed directly to the `style` attribute
     */
    size?: number | string;
    /**
     * Any of the named color palette options.
     */
    color?: BeaconColor;
  };

Do you think it should only accept named colors as a prop like shown in the code above or any color string as well (Hex, RGB...)?

@miukimiu
Copy link
Contributor

miukimiu commented Nov 7, 2022

Hi @dawitam11.

Just the named colors as you suggested would work. 👍🏽

The type can be called EuiBeaconColor instead of BeaconColor.

@qpointsystems
Copy link
Author

Fabulous :) thanks for this enhancement gang! Agree that the named colors is most appropriate as it constrains the choices to those of the theme. Going outside that list could/should involve going the 'extra step' and using CSS.

@dawitamene
Copy link
Contributor

@miukimiu Sorry for the slow response. I've been busy lately and was stuck with a documentation issue. I'll describe the issue in the PR and need your help on it.

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

Successfully merging a pull request may close this issue.

3 participants