Skip to content
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

AccordionTab cannot be used in custom wrapper #2052

Closed
tasirus opened this issue May 20, 2021 · 28 comments · Fixed by #5150
Closed

AccordionTab cannot be used in custom wrapper #2052

tasirus opened this issue May 20, 2021 · 28 comments · Fixed by #5150
Assignees
Labels
Component: Documentation Issue or pull request is related to Documentation
Milestone

Comments

@tasirus
Copy link

tasirus commented May 20, 2021

Current behavior
We use AccordionTab component in a custom wrapper (Accordion is wrapped as well). After recent change - f63a13a - accordion tabs are not displayed anymore because of type mismatch.

Expected behavior
We would like to still use Accordion in custom wrapper

Minimal reproduction of the problem with instructions
We have something like code below in our app:

import type { FunctionComponent } from 'react';
import type { AccordionTabProps } from 'primereact/accordion';
import { AccordionTab as PRAccordionTab } from 'primereact/accordion';

export type { AccordionTabProps };

export const AccordionTab: FunctionComponent<AccordionTabProps> = ({ children, ...otherProps }) => (
	<PRAccordionTab {...otherProps}>{children}</PRAccordionTab>
);

The type of our wrapper is not equal to AccordionTab so tabs are not rendered at all.

Please tell us about your environment:

  • React version: 17.0.2
  • PrimeReact version: 6.3.2
  • Browser: all

  • Language: TypeScript 4.2.4

@dattebayorob
Copy link

Same issue here

@mcandu mcandu added this to the 6.5.0 milestone Jul 6, 2021
@mcandu mcandu added the Status: Pending Review Issue or pull request is being reviewed by Core Team label Jul 6, 2021
@mertsincan mertsincan modified the milestones: 6.5.0, 7.0.0 Jul 16, 2021
@ShashankSuresh
Copy link

Same issue here

@Fallup
Copy link

Fallup commented Jan 12, 2022

Same problem here. This broke our app after upgrading.

@melloware
Copy link
Member

I wonder if instead of type == it needs to use instanceof instead

@kazamov
Copy link
Contributor

kazamov commented Jan 12, 2022

I wonder if instead of type == it needs to use instanceof instead

I think it will not help. The same issue with TabPanel component in the TabView.

@kazamov
Copy link
Contributor

kazamov commented Jan 12, 2022

It may help if your wrapper component is a class component that extends the AccordionTab or TabPanel. But if your wrapper comment is a function it will not help.

@inad9300
Copy link
Contributor

inad9300 commented Jul 1, 2022

Any progress on this front? Any known workarounds, perhaps?

@mertsincan
Copy link
Member

Hi,

Sorry for the delayed response! This structure has already changed. Please see;
https://github.com/primefaces/primereact/blob/master/components/lib/accordion/Accordion.js#L13

You can add __TYPE="AccordionTab" prop to your custom AccordionTab component. Exp;

<PRAccordionTab __TYPE="AccordionTab" {...otherProps}>...

I have tagged this issue as "Resolved" for now. If a new update comes from your side, please reopen this ticket. Thanks a lot for your understanding!

@mertsincan mertsincan removed this from the 8.2.0 milestone Jul 1, 2022
@mertsincan mertsincan removed the Status: Pending Review Issue or pull request is being reviewed by Core Team label Jul 1, 2022
@inad9300
Copy link
Contributor

inad9300 commented Jul 1, 2022

@mertsincan I don't manage to make that work: https://codesandbox.io/s/primereact-test-forked-drc2vg?file=/src/index.js

Having custom props is also not currently possible.

@inad9300
Copy link
Contributor

inad9300 commented Jul 4, 2022

@mertsincan @melloware sorry for the spam, but I'm not sure whether you got a notification from the previous message, since the ticket is closed and I only added the mention in an edit. Could you please confirm that you got my message, and maybe change the ticket's status? Thank you! 🙂

@mertsincan mertsincan reopened this Jul 4, 2022
@mertsincan
Copy link
Member

Hi @inad9300,

Sorry for the confusion! React provides us with methods that allow selecting only child elements. We do not know what will be returned from any function or class base components, and we cannot access the instances of the returned components from the DOM tree. This is about React. It allows us to reach only children with props.children. You can try this by creating a similar structure without the Accordion component.

@inad9300
Copy link
Contributor

inad9300 commented Jul 4, 2022

I'm sorry, I don't think I fully follow. Are you simply saying that fixing the problem is not so easy because of a limitation in React? Does this mean you will be working on the issue, or not?

My guess of where the problem lies would be that AccordionTab is not "self-rendered", but rather depends on Accordion to render it. It is basically a "non-component" (https://github.com/primefaces/primereact/blob/master/components/lib/accordion/Accordion.js#L6), which when used in isolation will not render anything to the screen. The rendering of AccordionTab should belong to AccordionTab, not to Accordion, and both should communicate based on a well-defined interface. That would be the first step towards solving the problem, as I see it. I can't quite picture a full solution at this moment.

@inad9300
Copy link
Contributor

inad9300 commented Jul 14, 2022

Did you have a chance to give this any more thought? 🙂

@melloware
Copy link
Member

@inad9300 I don't have it fully working but I have it "closer". Check out my example: https://codesandbox.io/s/primereact-test-forked-hh2sy5?file=/src/index.js

@inad9300
Copy link
Contributor

Thanks for sharing – great progress!

@inad9300
Copy link
Contributor

inad9300 commented Sep 12, 2022

@melloware How's the work on this going?

One thought I had about this topic is that a toggleable Panel component is basically an AccordionTab, except panels can already be extracted to separate components without a problem. From this point of view, one could implement an accordion based on panels. Maybe this is something that Accordion can do internally, or at least something that could be used as inspiration to improve Accordion.

Not to put any pressure on you, but it would be great if you could give an estimation of when you think this ticket might be addressed, independent of the particular solution. This is simply for me to decide whether to attempt my own accordion implementation based on panels, or to wait for PrimeReact's fix.

@melloware
Copy link
Member

@inad9300 I don't work for PrimeTek I am just an open source volunteer. If you need this level of support I suggest you look into PRO support.

I think this component would need to be refactored to make it possible.

@inad9300
Copy link
Contributor

Wow – first of all, props to you for all of your hard work!

That said, I find it a little concerning that a) you are not getting compensated for it (though I'm only assuming here, of course) and b) PrimeTek's work on PrimeReact seems negligible in comparison to your contributions. I mean, it would seem that you are the one sailing this boat! 😄

image

While PRO isn't for me yet, I did base my choice of PrimeReact on the fact that there was a company backing it, knowing that I could opt in to PRO if I ever needed it. But now I'm finding that maybe there isn't that much backing from PrimeTek's side? It'd be nice if PrimeTek could comment on this, particularly on how invested they are in PrimeReact (in general, and also versus their efforts in other frameworks).

@melloware
Copy link
Member

melloware commented Sep 22, 2022

@inad9300 PrimeReact is here to stay. I have Fortune 500 clients I use it at so I am personally vested in making sure its great and stays great. That being said PrimeTek has a lot of efforts going on and right now. They are updating PrimeVue for ARIA and once that is complete they are going to roll all those changes into PrimeReact 9. So, while it looks like there is no activity from PrimeTek they are quite active!

@mertsincan
Copy link
Member

mertsincan commented Sep 23, 2022

Hi @inad9300,

@melloware is a Member of the PrimeReact and PrimeFaces Teams. He really does great work. We are very happy to see him in the team. While making decisions about the changes to be made in the library, we talk within the team and implement accordingly and provide feedback on bugs. Accordingly, any team member can implement it. PrimeReact is currently managed by Melloware. Also, 2 more developers joined the PrimeReact team. So it's perfectly normal for everyone on the team to take on such a role. As you know, apart from community support, other PrimeReact products such as Layout, Designer and Blocks need to constantly renew themselves. New members of the team are currently taking part in these projects. They also really stepped us up on PRO support. As a result, our goal is always to optimize PrimeReact.
As Melloware said, we really care about Accessibility and are currently making a lot of changes to the components in PrimeVue. After we've finished them all, we'll export them to PrimeReact and PrimeNg projects as well. At this stage, you can see cases where different members of the team have undertaken to release the version :)

Let's get back to your problem. The __TYPE property is actually a property that should only be used to find the correct helper components. As you can see in Melloware's work, the returned by a custom component doesn't matter. Even removing it and just putting {props.children} will give the same result. As I said before, React only allows us to find the children of the parent component. It won't let us find it until his grandchildren. In short, we have no direct access to the parent's descendants. In order to get the result you want, AccordionTab needs to render itself. This means moving some code from Accordion to AccordionTab. This might make sense at first, but be aware of the changes in how such a change interacts with Accordion. Even moving some events there will require constant communication with both their children and siblings. We've tried this kind of experiment before with our other libraries and it was a really annoying process. It caused too many problems :) That's why all components of Prime* were designed that way.

Actually, as far as I understand in your topic, you want to design AccordionTab according to some special operations. I think you can create dynamic accordionTabs for this. Exp;
https://codesandbox.io/s/primereact-test-forked-1zfcqz?file=/src/index.js

@inad9300
Copy link
Contributor

Thanks a lot for the explanation; it gives me much peace of mind 🙂

It's good to know about the dynamic tabs possibility. In the end I went the route I hinted to in a previous comment and implemented what I wanted using toggleable panels, which are working well.

@mertsincan
Copy link
Member

Glad to hear, thanks a lot for the update! If you have any questions, please don't hesitate to ask. I'll try to answer before @melloware 🙂🙂

@Traveller23
Copy link

@mertsincan @melloware
I think this solution should be added to the documentation. Creating a variable number of tabs is a very common scenario.

@melloware melloware reopened this Oct 24, 2023
@melloware melloware self-assigned this Oct 24, 2023
@melloware melloware added the Component: Documentation Issue or pull request is related to Documentation label Oct 24, 2023
@github-actions github-actions bot added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Oct 24, 2023
@melloware melloware added this to the 10.0.6 milestone Oct 24, 2023
@melloware
Copy link
Member

I will add a showcase example @Traveller23

@adirzoari
Copy link

adirzoari commented Jul 7, 2024

any solution? @melloware @mertsincan @kazamov @Fallup @inad9300
m facing an issue where my custom FilterAccordionTab component, which extends PrimeReact's (V8) AccordionTab, is not rendering its content. However, when I directly use PrimeReact's AccordionTab within my custom FilterAccordion component, it works as expected.
FilterAccordion.tsx

import React from 'react';
import {
  Accordion as PrimeAccordion,
  AccordionTab as PrimeAccordionTab,
} from 'primereact/accordion';
import {
  AccordionProps as PrimeAccordionProps,
  AccordionTabProps as PrimeAccordionTabProps,
} from 'primereact/accordion';

// Extended Accordion props
export interface FilterAccordionProps extends PrimeAccordionProps {
  'data-testid'?: string;
}

// Extended AccordionTab props
export interface FilterAccordionTabProps extends PrimeAccordionTabProps {
  'data-testid'?: string;
}

// FilterAccordion component
export const FilterAccordion: React.FC<FilterAccordionProps> = ({
  children,
  'data-testid': dataTestId,
  ...props
}) => {
  return (
    <PrimeAccordion data-testid={dataTestId} {...props}>
      {children}
    </PrimeAccordion>
  );
};

// FilterAccordionTab component
export const FilterAccordionTab: React.FC<FilterAccordionTabProps> = ({
  children,
  'data-testid': dataTestId,
  ...props
}) => {
  return (
    <PrimeAccordionTab data-testid={dataTestId} {...props}>
      {children}
    </PrimeAccordionTab>
  );
};

Storybook:


import React, { useState } from 'react';
import { Meta, StoryObj } from '@storybook/react';
import {
  FilterAccordion,
  FilterAccordionTab,
  FilterAccordionProps,
} from '..';

type Story = StoryObj<FilterAccordionProps>;

const DefaultTemplate: Story = {
  render: (args) => {
    const [activeIndex, setActiveIndex] = useState<number>(0);

    return (
      <FilterAccordion
        {...args}
        activeIndex={activeIndex}
        onTabChange={(e: any) => setActiveIndex(e.index)}
      >
        <FilterAccordionTab header="Header I">Content I</FilterAccordionTab>
        <FilterAccordionTab header="Header II">Content II</FilterAccordionTab>
        <FilterAccordionTab header="Header III">Content III</FilterAccordionTab>
      </FilterAccordion>
    );
  },
};

export const Default: Story = {
  ...DefaultTemplate,
};

const PlaygroundTemplate: Story = {
  render: (args) => {
    const [activeIndex, setActiveIndex] = useState<number[]>([1, 2]);

    return (
      <FilterAccordion
        {...args}
        activeIndex={activeIndex}
        onTabChange={(e: any) => setActiveIndex([e.index])}
      >
        <FilterAccordionTab header="Header I">Content I</FilterAccordionTab>
        <FilterAccordionTab header="Header II">Content II</FilterAccordionTab>
        <FilterAccordionTab header="Header III">Content III</FilterAccordionTab>
      </FilterAccordion>
    );
  },
};

export const Playground: Story = {
  ...PlaygroundTemplate,
  args: {
    width: '280px',
  },
};

const meta: Meta<FilterAccordionProps> = {
  title: 'Table/Filters/Table Filter Accordion',
  component: FilterAccordion,
  tags: ['autodocs'],
  parameters: {
    docs: {
      description: {
        component: `A 'FilterAccordion' groups a collection of contents in tabs. It consists of one or more FilterAccordionProps elements which are collapsed by default.
        Tab to expand initially can be defined with the activeIndex property.`,
      },
    },
  },
};

export default meta;

Problem:

When using FilterAccordionTab inside FilterAccordion, the tabs do not render their content - I don't see nothing on the screen.

When using PrimeAccordionTab inside FilterAccordion, it works as expected.

What I Tried:

Verified that children are being passed correctly in both FilterAccordion and FilterAccordionTab.

Checked the structure and props of both FilterAccordionTab and PrimeAccordionTab.

Added console logs to check the rendering flow (but no logs are shown).

Expected Behavior: FilterAccordionTab should render the content just like PrimeAccordionTab.

Observed Behavior: FilterAccordionTab does not render any content inside FilterAccordion.

Additional Details:

Using PrimeReact for the accordion components.

The issue is only with the custom FilterAccordionTab.

Storybook setup is correctly configured and renders other components without issues.

Any insights or suggestions to resolve this would be greatly appreciated. Thank you!

@melloware
Copy link
Member

@adirzoari you can't as explained in the ticket the AccordionTab itself is not a component.

@adirzoari
Copy link

@melloware what the options I have? just map each component as in the example?

@melloware
Copy link
Member

Not sure without research or just go back to using a regular Accordion and AccordionTab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Documentation Issue or pull request is related to Documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.