fix(react): fix issue where accordion would not pass through props correctly #713
Conversation
|
Preview branch generated at https://accordion-fixes.d1gko6en628vir.amplifyapp.com |
| open={open} | ||
| onToggle={onToggle} | ||
| animationTiming={animationTiming} |
There was a problem hiding this comment.
We are already spreading {...panelElement.props}. Why not have these props be part of the panel's props (AccordionContentProps)?
There was a problem hiding this comment.
I realize that the AccordionProps extend from ExpandCollapsePanel, but since that wasn't actually implemented, I don't think it would be a breaking change to make AccordionContentProps extend from ExpandCollapsePanel instead
There was a problem hiding this comment.
Because they're 2 different elements. The resulting HTML looks something like this:
<div class="Accordion">
<button>...</button>
<div class="ExpandCollapse__Panel">...</div>
</div>The accordion props should apply to the .Accordion element. The panel props should apply to the .ExpandCollapse__Panel element. They need to be able to set html attributes independently of one another.
There was a problem hiding this comment.
I understand. This just seems set up a bit weird. For example, if I set an id on the AccordionContent element like this (note accordion-content-id):
return (
<Accordion open={open} onToggle={() => setIsOpen(!open)}>
<AccordionTrigger heading={{ level: 4 }}>{label}</AccordionTrigger>
<AccordionContent id="accordion-content-id">Here is some content</AccordionContent>
</Accordion>
);Then the rendered HTML has two elements with the same id, which is invalid HTML:
<div class="Accordion">
<h4>...</h4>
<div class="ExpandCollapse__panel" id="accordion-content-id">
<div class="Accordion__panel" id="accordion-content-id">Here is some content</div>
</div>
</div>Similarly, if I set any other attributes, such as className on the AccordionContent component, those attributes will be set on both the ExpandCollapse__panel and Accordion__panel elements.
Although I realize my original suggestion is not the correct solution, I still feel like something needs to change here.
There was a problem hiding this comment.
That's a pre-existing issue, I went ahead and created #714. We should definitely fix it, but I'm not sure what the solution should be yet and I don't want it blocking things I'm currently working on.
|
Preview branch generated at https://accordion-fixes.d1gko6en628vir.amplifyapp.com |
This is needed for https://github.com/dequelabs/axe-extension/issues/2476 and addresses some outstanding issues.