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

feat: add tag badge to palette picker #8208

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Dec 5, 2024

Summary

Adds an EuiBadge to the EuiColorPalettePicker list items. This allows tagging the main palette as Default without using parenthesis (i.e. Elastic (default)), see elastic/kibana#192848 (comment).

image

See storybook demo here.

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

  • 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
  • Release checklist
    • A changelog entry exists and is marked appropriately.

@nickofthyme nickofthyme requested a review from a team as a code owner December 5, 2024 21:39
cursor: default;
cursor: inherit;
Copy link
Contributor Author

@nickofthyme nickofthyme Dec 5, 2024

Choose a reason for hiding this comment

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

I changed this so that when the badge is placed inside a clickable component, such that clicking the badge will bubble up and trigger a click handler, the cursor should adopt to the parent component style by default. If the badge itself is clickable, this cursor style is overridden.

@cee-chen
Copy link
Contributor

cee-chen commented Dec 6, 2024

Rather than add an API for an extremely specific use-case, why not make title more flexible and allow it to render a React.JSX.Element? Consumers who need a badge can then then render their own flex wrappers etc as needed.

If we want to put some guard rails around custom renders, I'd also be open to a more limited titlePrepend/titleAppend API instead, which would allow for more than just badges (e.g. icons, etc.)

@nickofthyme
Copy link
Contributor Author

nickofthyme commented Dec 6, 2024

Ok I made the title to be a ReactNode instead, see e6813a0.

@MichaelMarcialis
Copy link
Contributor

MichaelMarcialis commented Dec 9, 2024

@nickofthyme: Apologies for sticking my nose into this PR. Regardless of the technical implementation, I'm personally not a fan of appending an EuiBadge to indicate the default. The badge has a thicker font weight than the actual title of the color palette and is given far too heavy of an emphasis. I'd much prefer sticking with a basic parenthetical (default), or perhaps giving it a similar treatment to form label appends (quick example below).

CleanShot 2024-12-09 at 13 14 53

Thoughts?

CCing @gvnmagni.

@nickofthyme
Copy link
Contributor Author

nickofthyme commented Dec 9, 2024

No problem at all, happy to get your thoughts. I like your proposal better. Updated in fb1b41d.

image

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

Comment on lines +165 to +175
{typeof title !== 'string' ? (
title
) : (
<EuiText
aria-hidden="true"
className="euiColorPalettePicker__itemTitle"
size="xs"
>
{title}
</EuiText>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to be an annoying jerk here - based on Marcialis's mocks, I'd rather add new prepend/append APIs over allowing title to be a ReactNode 🙈 It has the added benefit of less string/type shenanigans, and also matching existing APIs in EuiComboBox/EuiSelectable.

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.

5 participants