Capgemini / dcx-react-library

React Library UI/UX agnostic
https://main--6069a6f47f4b9f002171f8e1.chromatic.com
MIT License
107 stars 7 forks source link

Paragraph - enable paragraph to have a custom content #566

Closed daniele-zurico closed 8 months ago

daniele-zurico commented 8 months ago

Currently, the paragraph component allows only to have a value however it should allow it to contain custom content something like:

<Paragragraph>
   <ImCustomComponent>
</Paragraph>

The current paragraph however is already used in production so we can't remove the value property the only thing we can do is to extend the use by adding this extra ability. We need to make sure however that is you using the new function you not using the old one so something like this:

<Paragragraph value="im the value">
   <ImCustomComponent>
</Paragraph>

will not happen.

daniele-zurico commented 8 months ago

an Idea on how it can be implemented

type BaseProps = {
  test: string;
  a?: string;
  b?: string;
};

type PropsA = BaseProps & { a: string };
type PropsB = BaseProps & { b: string };

type Props = PropsA | PropsB;

function Component(props: Props) {
  if (props.a) {
    // do something
  }

  if (props.b) {
    // do something
  }

  return null;
} 
zweertsk commented 8 months ago

This looks like arbitrary composition. Have the Paragraph accept a prop that is either of type 'value' or another component.

function Component({value, children}) {

    return (
        <Paragraph value={<AnotherComponent />} />
        <Paragraph value={"A string as p text"} />
    )
}
jgonza16 commented 8 months ago

you suggest only one way with prop value. this approach makes the component less complex but i feel more natural the paragraph works like higher-order component. I think we will use this more.

<Paragraph>
  This is the simple custom-content of the
  <strong>
    paragraph
  </strong>
</Paragraph>

@kjzweerts you can review https://github.com/Capgemini/dcx-react-library/pull/567 and you get feedback.

daniele-zurico commented 8 months ago

hi @kjzweerts thanks for your comment and I think your point is valid however I do agree with @jgonza16 .... it will looks more natural to use the component like an html native component:

<Paragraph>content</Paragraph>

the only reason we want to leave value property there is because some other projects are already using the Paragraph and we don't want to introduce a breaking change.

On the other side can I ask you if you using the library? just out of curiosity

zweertsk commented 8 months ago

On the other side can I ask you if you using the library? just out of curiosity

No I am not a user. I am in an Angular project and my react knowledge is starting to get vague 😅. So I’m maybe looking to contribute to have best of both worlds.

I work for Sogeti and found this when browsing GitHub.

daniele-zurico commented 8 months ago

absolutely @kjzweerts ... let me know when you want to get involved and in which way and we can discuss a ticket as starting point. Feel free to contact me on msTeam

daniele-zurico commented 8 months ago

Closed by https://github.com/Capgemini/dcx-react-library/pull/567