feat(component): add AccordionPanel component

[bc-athorne]

What?

Add AccordionPanel and Accordion components to BigDesign

Why?

CHI-1984 - This component is needed for current Feedonomics work by CMX team.

Screenshots/Screen Recordings

Component

Screen.Recording.2022-08-10.at.11.59.31.AM.mov

Docs

Testing/Proof

All unit tests are passing.

[MariaJose]

Lgtm 👏🏼 ! Just few comments

[MariaJose]

I'm not sure why this test is failing in CircleCI. It logs the correct value and passes in my local environment.

Local env:

test('if defaultExpanded is set to true, accordion is expanded', async () => {
  render(
    <AccordionPanel defaultExpanded={true} header="Panel Header" isExpanded onClick={handleClick}>
      <Text>This is a child component</Text>
    </AccordionPanel>,
  );

  const panel = await screen.findByRole('button');

  console.log(panel.getAttribute('aria-expanded'));

  expect(panel.getAttribute('aria-expanded')).toBe('true');
});

It failed for me locally as well :eyes. But I add this and the test passed :

const [isExpanded, setisExpanded] = useState(defaultExpanded ?? false);

I think that we might not need this since a default value should not be changed?, unless missing something 🤔 :

useEffect(() => {
   if (defaultExpanded) {
     setisExpanded(defaultExpanded);
   }
 }, [defaultExpanded]);

[chanceaclark]

Hey @bc-athorne! Congrats opening up your first contribution to BigDesign! I do have a few comments we should address, including some API updates we should make. Let me know if you have any questions!

FYI I'm just requesting changes to make sure we address some of the API concerns before we merge.

[BC-Turner]

Couple comments:

1. When collapsing the component, it looks like the 2nd child overlaps the focused state of the first. Is there a way to bring that focused state forward so we don't lose part of the stroke?

2. For the implementation example, let's use either text or radio buttons for both panels and have either icons on both or none. I think we don't want users mixing panels of different content and keeping consistent with either having an icon on all or no icons. Just to avoid confusion.

Otherwise it looks great!

[rtalvarez]

Sorry kinda late to the party here, but this is looking great so far

[jorgemoya]

Looking good! Thanks for working on this Amanda. Left some comments, let me know what you think.

[jorgemoya]

Should this actually extend from PanelProps? 🤔

[bc-athorne]

Can you provide some clarification on why you suggest this?

[chanceaclark]

Side bar from Jorges comment, do we even need consumers to pass in HTMLDivElement attributes? If we leave this out right now, we can always add back in when consumers need it. We can't remove it without a breaking change if we add it in now.

[BC-Turner]

Looks awesome! Great job, Amanda!

[chanceaclark]

I have a few more comments for ya @bc-athorne but thanks for making those updates!

[MariaJose]

Looking good! 👏🏼 , just left a few comments

[rtalvarez]

🐑

[MariaJose]

Lgtm 👍🏼 !

[MariaJose]

I think we could remove onClick ?

[chanceaclark]

And isExpanded

[bc-athorne]

Do we want to remove isExpanded? I thought this was needed if we wanted this to be uncontrolled / controlled. Although, I'm not 100% clear on that.

[chanceaclark]

Handling the state inside the component makes it a controlled component, which you are doing with this hook: https://github.com/bigcommerce/big-design/pull/933/files#diff-5fc0e597eae1361cf0fba2493f91154fba01f35b851baace00572488ec7ec010R22

If we want consumers to control the state, we'd elevate that outside of the component and let consumers pass it in.

[rtalvarez]

I don't think this is entirely accurate -- this component is purposefully meant to be a controlled component, hence the ability for consumers to provide isExpanded and onClick, and we are definitely elevating these as far as I can see (through the panel items). We should keep isExpanded and onClick IMO

We should, however, export the useAccordion hook (I now realize its not being exported, maybe this is the source of the disconnect?), so consumers can opt for controlled or uncontrolled accordions.

[bc-athorne]

Sorry for the confusion 🤦‍♀️. I moved useAccordion outside of the component.

[chanceaclark]

The problem I see if that those props are not even being used, so we either need to use them or lose them.

[rtalvarez]

We could drop onClick, but isExpanded is literally a required prop, it will get used. Not sure I follow

[chanceaclark]

What I mean is when you pull down the component, the passed in props.isExpanded and props.onClick are technically not being used/consumed within the component. onClick only works because we are spreading it into <Box {...props}> and the handler is being called when you expand/collapse the accordion. The passed props.isExpanded though is not being consumed anywhere, the state is being respected only through the state returned from useAccordion.

Note: we technically can't have an uncontrolled component here unless we use the native HTML details/summary elements. State will have to be managed by our component.

[rtalvarez]

Was chatting with Chance this morning in the office, and cleared up some of the misunderstandings in this thread. The issues, as they stand right now, are:

• The component is not honoring the provided isExpanded / onClick props. To correctly visualize this, we can think of a consumer that programatically mutates this prop. Since it's not going through onClick, it will not work
• Because of the above, the useAccordion hook kinda operates on the item level, but the exported element is the group level. This also means we only need to export 1 component (the group one)
• The Hooks folder is not being exported, so we need to move the useAccordion hook to the component folder and probably export it from there
• Some of the names could be tweaked to make more sense, stylistic changes, etc

We can sync offline and discuss these issues, IMO we'll need to address them before we move forward

[chanceaclark]

Side bar from Jorges comment, do we even need consumers to pass in HTMLDivElement attributes? If we leave this out right now, we can always add back in when consumers need it. We can't remove it without a breaking change if we add it in now.

[bc-juanvasquez]

have we tested nested children? Not sure if memo would interfere here with the diffing algo..

Need some changes -- discuss offline

[chanceaclark]

One small little fix, otherwise looks good from my end.

[chanceaclark]

Maybe we should just assign this to a local const instead of using this type of JavaScript sorcery.

Suggested change

	({ panels } = useAccordionPanel([
	{
	header: 'Panel Header',
	children: <Text>This is a child component</Text>,
	},
	]));
	const { panels } = useAccordionPanel([
	{
	header: 'Panel Header',
	children: <Text>This is a child component</Text>,
	},
	]);

Same below...

[rtalvarez]

Thanks for all the changes 🙏