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 #567

Closed jgonza16 closed 8 months ago

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (5c8aa34) 100.00% compared to head (5423aab) 100.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## release/1.0.0 #567 +/- ## =============================================== Coverage 100.00% 100.00% =============================================== Files 67 67 Lines 1240 1247 +7 Branches 458 460 +2 =============================================== + Hits 1240 1247 +7 ``` | [Files](https://app.codecov.io/gh/Capgemini/dcx-react-library/pull/567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Capgemini) | Coverage Δ | | |---|---|---| | [src/paragraph/Paragraph.tsx](https://app.codecov.io/gh/Capgemini/dcx-react-library/pull/567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Capgemini#diff-c3JjL3BhcmFncmFwaC9QYXJhZ3JhcGgudHN4) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jgonza16 commented 8 months ago

hello @daniele-zurico, changes is done. I have made value take precedence over children because it is the previous property, how do you see it?

daniele-zurico commented 8 months ago

hello @daniele-zurico, changes is done. I have made value take precedence over children because it is the previous property, how do you see it?

hey @jgonza16 first thing first thanks a lot for your speedy PR it is really solid. I just left few comments most of them can be directly committed without you to copy the content of it. The main observation I put in your PR is the fact that at least one between content and custom content need to be present in the Paragraph component for two reasons:

  1. before value was mandatory so it was expected to have a value whilst now we moved to optional
  2. a Paragraph without content os not a paragraph. Let me know if you agree on that
jgonza16 commented 8 months ago

@daniele-zurico yes you are right. we can use typescript better for this. we pushed changes in paraagraph.tsx. this is my approach:

type Props = {
  className?: string;
  value: string | number;
  children: string | number | JSX.Element;
  props?: React.HTMLAttributes<HTMLParagraphElement>;
};

type ParagraphValue = Omit<Props, 'children'>;
type ParagraphChildren = Omit<Props, 'value'>;
type ParagraphProps = ParagraphValue | ParagraphChildren;

const isValueType = (p: any): p is ParagraphValue => !!p.value;
const isChildrenType = (p: any): p is ParagraphChildren => !!p.children;

into the compoent check:

export const Paragraph = ({
  className,
  props,
  ...rest
}: ParagraphProps): JSX.Element => {

  if (isChildrenType(rest)) content = rest.children;
  if (isValueType(rest)) content = rest.value;

};
daniele-zurico commented 8 months ago

@daniele-zurico yes you are right. we can use typescript better for this. we pushed changes in paraagraph.tsx. this is my approach:

type Props = {
  className?: string;
  value: string | number;
  children: string | number | JSX.Element;
  props?: React.HTMLAttributes<HTMLParagraphElement>;
};

type ParagraphValue = Omit<Props, 'children'>;
type ParagraphChildren = Omit<Props, 'value'>;
type ParagraphProps = ParagraphValue | ParagraphChildren;

const isValueType = (p: any): p is ParagraphValue => !!p.value;
const isChildrenType = (p: any): p is ParagraphChildren => !!p.children;

into the compoent check:

export const Paragraph = ({
  className,
  props,
  ...rest
}: ParagraphProps): JSX.Element => {

  if (isChildrenType(rest)) content = rest.children;
  if (isValueType(rest)) content = rest.value;

};

hey @jgonza16 we can go through this approach as well (that is the original I suggested) however I didn't progress with that because I don't know how the storybook will behave with the 3 types on the following page: https://main--6069a6f47f4b9f002171f8e1.chromatic.com/?path=/docs/dcxlibrary-typography-paragraph-documentation--docs (on the table at the end of the page) I think it will not be able to display that and will be missed in the documentation.