Skip to content

feat: accordion header tag (#4999) #5424

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
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: accordion header tag (#4999) #5424

wants to merge 6 commits into from

Conversation

@michaelokolo
Copy link

@michaelokolo michaelokolo commented Oct 14, 2025

Description

This pull request introduces a new feature that allows changing the heading tag of an Accordion Item Header.

Related issue: #4999

Changes

  1. Updated metaAccordionHeader in accordion.ws.ts to include h1–h6 options, enabling users to select the heading level in the Item Header settings.
  2. Updated AccordionHeader in accordion.tsx to accept and render h1–h6 tags dynamically.
  3. Moved the margin-top and margin-bottom styles from metaAccordionHeader to AccordionHeader to ensure consistent spacing across all heading levels.

How to Test

  1. Open any project.
  2. Add an Accordion to the canvas.
  3. Click an Accordion Item Header. In the right-hand settings panel, you should see a new Tag dropdown with options h1–h6.
  4. Select different tags and verify the rendered heading levels in the browser’s developer tools.

h2
h5
Tag

@michaelokolo michaelokolo changed the title Feat/accordion header tag 4999 feat/accordion header tag 4999 Oct 14, 2025
@michaelokolo michaelokolo changed the title feat/accordion header tag 4999 feat: accordion header tag 4999 Oct 14, 2025
@michaelokolo michaelokolo changed the title feat: accordion header tag 4999 feat: accordion header tag (#4999) Oct 14, 2025
TrySound
TrySound reviewed Oct 15, 2025
View reviewed changes
packages/sdk-components-react-radix/src/accordion.tsx Outdated
type Props = ComponentPropsWithoutRef<typeof Header> & { tag?: Tag };
export const AccordionHeader = forwardRef<HTMLHeadingElement, Props>(
({ tag: legacyTag, ...props }, ref) => {
const tag = getTagFromProps(props) ?? legacyTag ?? defaultTag;
Copy link
Member

@TrySound TrySound Oct 15, 2025

Choose a reason for hiding this comment

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

Call variable in pascal case "Heading" and you can use it in jsx without createElement which is considered legacy.

Copy link
Author

@michaelokolo michaelokolo Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Alright, I will resolve this. Thank you.

Copy link
Author

@michaelokolo michaelokolo Oct 15, 2025

Choose a reason for hiding this comment

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

Hello @TrySound , I’ve made the necessary updates based on your earlier comments.

TrySound
TrySound reviewed Oct 15, 2025
View reviewed changes
packages/sdk-components-react-radix/src/accordion.tsx Outdated
Comment on lines 65 to 69
style: {
...(props.style || {}),
marginTop: 0,
marginBottom: 0,
},
Copy link
Member

@TrySound TrySound Oct 15, 2025

Choose a reason for hiding this comment

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

Builder does not support "style" property. So please remove it. Otherwise it will override user preferences from tokens.

Copy link
Author

@michaelokolo michaelokolo Oct 15, 2025

Choose a reason for hiding this comment

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

Alright, I will resolve this too. There is a margin-top: 0 and margin-bottom: 0 applied to the H3 Item Header in accordion.ws.ts, which is why I applied the style here. I wanted to ask, do we still need these styles?

Copy link
Member

@TrySound TrySound Oct 15, 2025

Choose a reason for hiding this comment

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

Yes, I think they still needed. At least to not break existing projects.

Copy link
Author

@michaelokolo michaelokolo Oct 15, 2025

Choose a reason for hiding this comment

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

And just to confirm, do we need this margin-top: 0 and margin-bottom: 0 for all heading levels or just for H3? I am planning to apply it to all heading levels in accordion.ws.ts.

Copy link
Author

@michaelokolo michaelokolo Oct 21, 2025

Choose a reason for hiding this comment

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

@TrySound the PR was updated last week, I just didn’t drop a message here.

@michaelokolo michaelokolo force-pushed the feat/accordion-header-tag-4999 branch from 179b2b1 to fd35c7f Compare October 15, 2025 22:53
@michaelokolo michaelokolo force-pushed the feat/accordion-header-tag-4999 branch from fd35c7f to 398a540 Compare October 15, 2025 23:01
@michaelokolo michaelokolo requested a review from TrySound October 22, 2025 20:14
TrySound
TrySound reviewed Oct 24, 2025
View reviewed changes
packages/sdk-components-react-radix/src/accordion.tsx Outdated
type Tag = "h1" | "h2" | "h3" | "h4" | "h5" | "h6";
type Props = ComponentProps<typeof Header> & { tag?: Tag };
export const AccordionHeader = forwardRef<HTMLHeadingElement, Props>(
({ tag: legacyTag, ...props }, ref) => {
Copy link
Member

@TrySound TrySound Oct 24, 2025

Choose a reason for hiding this comment

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

Since this component never accepted tag property there is no need to handle legacy version. tag property you added in meta will add control which will write directly into this call getTagFromProps(props). So just remove "tag" property from here and all will continue to work.

Copy link
Author

@michaelokolo michaelokolo Oct 24, 2025

Choose a reason for hiding this comment

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

@TrySound The tag property has been removed, and defaultTag is now set to h1 to ensure the rendered tag stays in sync with properties like data-ws-editable-placeholder from the moment the component is added.

@michaelokolo michaelokolo force-pushed the feat/accordion-header-tag-4999 branch from 398a540 to 51d10c8 Compare October 24, 2025 14:35
TrySound
TrySound approved these changes Oct 24, 2025
View reviewed changes
Copy link
Member

@TrySound TrySound left a comment

Choose a reason for hiding this comment

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

Great work! Thanks

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

Reviewers

@TrySound TrySound TrySound approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@michaelokolo @TrySound