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.

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

feat: add append option to palette picker options #8208

Merged
merged 15 commits into from
Jan 16, 2025
Merged

feat: add append option to palette picker options #8208

merged 15 commits into from
Jan 16, 2025

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Dec 5, 2024 •
edited
Loading

Summary

Updated EuiColorPalettePicker - adds append to EuiColorPalettePickerPaletteProps to support appending custom content to the title. This allows tagging the main elastic 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
nickofthyme
nickofthyme commented Dec 5, 2024
View reviewed changes
packages/eui/src/components/badge/badge.styles.ts
cursor: default;
cursor: inherit;
Copy link
Contributor Author

@nickofthyme nickofthyme Dec 5, 2024 •
edited
Loading

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 •
edited
Loading

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

@MichaelMarcialis
Copy link
Contributor

MichaelMarcialis commented Dec 9, 2024 •
edited
Loading

@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 •
edited
Loading

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

image

@nickofthyme nickofthyme requested a review from cee-chen January 11, 2025 00:03
@mgadewoll
Copy link
Contributor

@nickofthyme Fyi, I'll be taking over as reviewer on this 🙂

@mgadewoll mgadewoll requested review from mgadewoll and removed request for cee-chen January 13, 2025 09:20
mgadewoll
mgadewoll reviewed Jan 14, 2025
View reviewed changes
Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

I think we can also update this section in our EUI docs to mention the possibility to add custom content and add an append to one option for the examples:

  • packages/eui/src-docs/src/views/color_picker/color_picker_example.js
  • packages/website/docs/components/forms/color_selection.mdx

packages/eui/src/components/color_picker/color_palette_picker/color_palette_picker.stories.tsx Outdated Show resolved Hide resolved
packages/eui/src/components/color_picker/color_palette_picker/color_palette_picker.stories.tsx Outdated Show resolved Hide resolved
packages/eui/src/components/color_picker/color_palette_picker/color_palette_picker.stories.tsx Outdated Show resolved Hide resolved
packages/eui/changelogs/upcoming/8208.md Outdated Show resolved Hide resolved
packages/eui/src/components/color_picker/color_palette_picker/color_palette_picker.tsx Show resolved Hide resolved
@nickofthyme nickofthyme requested a review from mgadewoll January 14, 2025 16:24
mgadewoll
mgadewoll reviewed Jan 15, 2025
View reviewed changes
Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

@nickofthyme I took the liberty to add the mentioned changes to your branch. I hope you don't mind.

Additionally I ran snapshot updates to unblock the pipeline and ran VRT to add images for the newly added story.

packages/eui/src/components/color_picker/color_palette_picker/color_palette_picker.tsx Outdated Show resolved Hide resolved
packages/website/docs/components/forms/color_selection.mdx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mgadewoll

mgadewoll
mgadewoll approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ Thanks for the contribution and your patience! 🎉

@mgadewoll mgadewoll merged commit 6a20937 into elastic:main Jan 16, 2025
5 checks passed
@nickofthyme nickofthyme deleted the palette-tagging branch January 16, 2025 14:57
@acstll acstll mentioned this pull request Jan 22, 2025
Open
acstll added a commit to acstll/kibana that referenced this pull request Jan 22, 2025
@acstll
Update COLOR_PALETTE type
a56f391 
to account for the `title` being mandatory on EuiColorPalettePickerPaletteProps
See elastic/eui#8208 (comment)
@nickofthyme nickofthyme changed the title feat: add tag badge to palette picker feat: add append option to palette picker options Jan 22, 2025
@nickofthyme nickofthyme mentioned this pull request Jan 22, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen left review comments

@mgadewoll mgadewoll mgadewoll approved these changes

Assignees

@mgadewoll mgadewoll

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nickofthyme @cee-chen @MichaelMarcialis @mgadewoll @kibanamachine @elasticmachine