-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
base: main
Are you sure you want to change the base?
feat: accordion header tag (#4999) #5424
Conversation
type Props = ComponentPropsWithoutRef<typeof Header> & { tag?: Tag }; | ||
export const AccordionHeader = forwardRef<HTMLHeadingElement, Props>( | ||
({ tag: legacyTag, ...props }, ref) => { | ||
const tag = getTagFromProps(props) ?? legacyTag ?? defaultTag; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
style: { | ||
...(props.style || {}), | ||
marginTop: 0, | ||
marginBottom: 0, | ||
}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
179b2b1
to
fd35c7f
Compare
fd35c7f
to
398a540
Compare
Description
This pull request introduces a new feature that allows changing the heading tag of an Accordion Item Header.
Related issue: #4999
Changes
metaAccordionHeader
inaccordion.ws.ts
to includeh1
–h6
options, enabling users to select the heading level in the Item Header settings.AccordionHeader
inaccordion.tsx
to accept and renderh1
–h6
tags dynamically.margin-top
andmargin-bottom
styles frommetaAccordionHeader
toAccordionHeader
to ensure consistent spacing across all heading levels.How to Test