Skip to content

feat(Accordion): support emoji icon#1492

Open
kevinports wants to merge 6 commits into
nextfrom
feat/accordion/support-emoji
Open

feat(Accordion): support emoji icon#1492
kevinports wants to merge 6 commits into
nextfrom
feat/accordion/support-emoji

Conversation

@kevinports

@kevinports kevinports commented May 27, 2026

Copy link
Copy Markdown
Contributor
🎫 Resolve RM-16937

🎯 What does this PR do?

Adds emoji icon support to the Accordion component alongside existing Font Awesome icon support.

This is necessary because the new WYSIWYG accordion UI in our editor supports selecting both Font Awesome and emoji icons https://github.com/readmeio/readme/pull/19025

🧪 QA tips

Paste each snippet into a ReadMe doc and confirm the accordion renders correctly.

Emoji icon

<Accordion icon="🚀" title="Emoji Icon">
Content inside the accordion.
</Accordion>
  • A 🚀 emoji appears to the left of the title
  • The accordion opens/closes on click

Font Awesome icon (existing behavior, no regression)

<Accordion icon="fa-book" title="FA Icon">
Content inside the accordion.
</Accordion>
  • A duotone book icon appears to the left of the title

Font Awesome icon with color (no regression)

<Accordion icon="fa-star" iconColor="red" title="Colored Icon">
Content inside the accordion.
</Accordion>
  • Icon renders in red; emoji icons are unaffected by iconColor

No icon (no regression)

<Accordion title="No Icon">
Content inside the accordion.
</Accordion>
  • No icon element rendered, title appears alone

📸 Screenshot or Loom

Comment thread components/Accordion/index.tsx Outdated
Comment on lines +5 to +6
/** @see https://docs-v5.fontawesome.com/web/reference-icons */
const FA_PREFIXES = new Set(['fab', 'fad', 'far']);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We use the @ui/EmojiPicker component in the Accordion wysiwyg block in the MdxishEditor in the monorepo. @ui/EmojiPicker allows a user to select between solid and duotone font awesome variants and will emit values with these class prefixes to differentiate those variants.

So the icon prop could receive values like fad fa-smile or far fa-smile instead of just fa-smile.

Accounting for that here and falling back to duotone if no prefix is provided.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we not need to support fal and fas as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah there a bunch of possible classes but I'm matching what our UI sets here https://github.com/readmeio/readme/blob/next/packages/react/src/ui/EmojiPicker/index.tsx#L23

@kevinports kevinports marked this pull request as ready for review May 27, 2026 20:19

@maximilianfalco maximilianfalco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm! tested this both in the monorepo and the demo app and they both work nicely! just one non blocking question about the FA prefixes!

Comment thread components/Accordion/index.tsx Outdated
Comment on lines +5 to +6
/** @see https://docs-v5.fontawesome.com/web/reference-icons */
const FA_PREFIXES = new Set(['fab', 'fad', 'far']);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we not need to support fal and fas as well?

@eaglethrost eaglethrost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kevinports Did some quick fontawesome research and I think you need to also include 'fas', 'fal', 'fat' in the FA_PREFIXES as well as it won't render the icon anymore.

Given these accordions:

<Accordion icon="fas fa-star" title="Solid star (fas)">
</Accordion>

<Accordion icon="fal fa-book" title="Light book (fal)">
</Accordion>

<Accordion icon="fat fa-bell" title="Thin bell (fat)">
</Accordion>

Before:
Screenshot 2026-05-28 at 11 20 23 am

After:
Screenshot 2026-05-28 at 11 20 31 am

Other than that, the changes work nicely!

<details className="Accordion" onToggle={() => setIsOpen(!isOpen)}>
<summary className="Accordion-title">
<i className={`Accordion-toggleIcon${isOpen ? '_opened' : ''} fa fa-regular fa-chevron-right`}></i>
{icon && <i className={`Accordion-icon fa-duotone fa-solid ${icon}`} style={{ color: `${iconColor}` }}></i>}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it fine to not have the fa-duotone class anymore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep it's replaced with the fad ${icon} class from L15 above. fad is the shorthand for fa-dutotone.

@kevinports

Copy link
Copy Markdown
Contributor Author

@eaglethrost I made some bigger changes after realizing we'll need to support the emoji & different font-awesome icons across other built-in components as well.

@kevinports kevinports requested a review from eaglethrost June 4, 2026 18:46
eaglethrost
eaglethrost previously approved these changes Jun 5, 2026

@eaglethrost eaglethrost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep changes looks good to me!

@kevinports kevinports requested review from a team, darrenyong and flinehan June 5, 2026 15:16
@kevinports

Copy link
Copy Markdown
Contributor Author

@eaglethrost wanted to also call out my comment here #1498 (review)

We should probably take a pass at verifying that these changes dont cause any regressions for users with custom css targeting the current icon DOM and classnames the engine compiles to! Otherwise I think this is ready to rock and roll.

@gkoberger gkoberger force-pushed the feat/accordion/support-emoji branch from 82fec06 to fa33152 Compare June 7, 2026 00:20
@jboyens jboyens force-pushed the feat/accordion/support-emoji branch from fa33152 to 82fec06 Compare June 8, 2026 18:00
@eaglethrost

eaglethrost commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

@eaglethrost wanted to also call out my comment here #1498 (review)

We should probably take a pass at verifying that these changes dont cause any regressions for users with custom css targeting the current icon DOM and classnames the engine compiles to! Otherwise I think this is ready to rock and roll.

Yep! Overall the changes here are fine because we're still using <i> to render Icons & the base classes are already passed in, so I anticipate it'll be fine for most cases. But there's 2 specific CSS rule that'll break:

  1. If the rule refers to the now removed --Card-icon-color style variable
  2. If the rule specifically targets the now removed fa-duotone or fa-solid class attributes

I think it should be unlikely to happen, users would need to inspect the HTML to be aware of them, but I could see it happen occasionally, so I think this is fine but we should keep an eye out.

Update: I think 1) looks like a niche case, but we should address 2), so I've reused the current fa-duotone and fa-solid classes to maintain the same class

@eaglethrost eaglethrost requested review from davinhazard and removed request for darrenyong and flinehan June 9, 2026 07:45
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.

4 participants