feat: migrate typography #2677
feat: migrate typography #2677
Conversation
vishvamsinh28
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
components/typography/Heading.tsx
Outdated
| level?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'; | ||
| textColor?: string; | ||
| className?: string; | ||
| children: React.ReactNode; |
There was a problem hiding this comment.
| children: React.ReactNode; | |
| children?: React.ReactNode; |
components/typography/Paragraph.tsx
Outdated
| textColor?: string; | ||
| fontWeight?: string; | ||
| className?: string; | ||
| children: React.ReactNode; |
There was a problem hiding this comment.
| children: React.ReactNode; | |
| children?: React.ReactNode; |
components/typography/TextLink.tsx
Outdated
| href: string; | ||
| className?: string; | ||
| target?: string; | ||
| children: React.ReactNode; |
There was a problem hiding this comment.
| children: React.ReactNode; | |
| children?: React.ReactNode; |
There was a problem hiding this comment.
Hi @vishvamsinh28, kindly use Enums for those values which are constant, instead of comparing it with string. It will be then easier for developers to get those values separately. You can define a separate type declaration file to specify enums for different files of this directory.
components/typography/Heading.tsx
Outdated
| typeStyle?: | ||
| | 'heading-xl' | ||
| | 'heading-lg' | ||
| | 'heading-md' | ||
| | 'heading-md-semibold' | ||
| | 'heading-sm' | ||
| | 'heading-sm-semibold' | ||
| | 'heading-xs' | ||
| | 'heading-xs-semibold' | ||
| | 'body-lg' | ||
| | 'body-md' | ||
| | 'body-sm'; |
There was a problem hiding this comment.
instead of giving the values in string, you should use the enum property inside typescript.
components/typography/TextLink.tsx
Outdated
| const classNames = twMerge(`text-secondary-500 underline hover:text-gray-800 font-medium transition ease-in-out duration-300 ${className || ''}`); | ||
|
|
||
| return ( | ||
| <a |
There was a problem hiding this comment.
We should use link tag also here as previously used in js file
There was a problem hiding this comment.
i tried adding that but shows some kind of an error
"Server Error
Error: Invalid with child. Please remove or use .
Learn more: https://nextjs.org/docs/messages/invalid-new-link-with-extra-anchor"
There was a problem hiding this comment.
So, the solution is, replace a with Link tag here and add the same CSS properties inside span as child element.
|
@akshatnema @sambhavgupta0705 requested changes have been made |
components/typography/Heading.tsx
Outdated
| enum HeadingTypeStyle { | ||
| xl = 'heading-xl', | ||
| lg = 'heading-lg', | ||
| md = 'heading-md', | ||
| md_semibold = 'heading-md-semibold', | ||
| sm = 'heading-sm', | ||
| sm_semibold = 'heading-sm-semibold', | ||
| xs = 'heading-xs', | ||
| xs_semibold = 'heading-xs-semibold', | ||
| body_lg = 'body-lg', | ||
| body_md = 'body-md', | ||
| body_sm = 'body-sm', | ||
| } | ||
|
|
||
| enum HeadingLevel { | ||
| h1 = 'h1', | ||
| h2 = 'h2', | ||
| h3 = 'h3', | ||
| h4 = 'h4', | ||
| h5 = 'h5', | ||
| h6 = 'h6', | ||
| } |
There was a problem hiding this comment.
Specify all the enums in different type definition file.
components/typography/Heading.tsx
Outdated
| xl = 'heading-xl', | ||
| lg = 'heading-lg', | ||
| md = 'heading-md', | ||
| md_semibold = 'heading-md-semibold', |
There was a problem hiding this comment.
you should use camelCase naming convention here, because now these are constants and will be used same as variables.
components/typography/Paragraph.tsx
Outdated
| } | ||
|
|
||
| return ( | ||
| <p data-testid="Paragraph-test" className={`${textColor} ${classNames}`}>{children}</p> |
There was a problem hiding this comment.
Kindly add twMerge in the className here to provide correct classNames.
components/typography/Heading.tsx
Outdated
| } | ||
|
|
||
| return ( | ||
| <Tag className={`${textColor} ${classNames}`} id={id}> |
|
@akshatnema requested changes are made |
components/typography/Paragraph.tsx
Outdated
| } | ||
|
|
||
| return ( | ||
| <p data-testid="Paragraph-test" className={twMerge(textColor, classNames)}>{children}</p> |
There was a problem hiding this comment.
can you please explain this datat-testId
There was a problem hiding this comment.
I know it is of no use as of now and I removed it when I created the components. However, once the migration for all components is completed, we will add tests, so I left the data-testid as it. Should I remove it ?
There was a problem hiding this comment.
If you are adding data-testid, it's good because later on it will be easier for us to integrate the tests related to it.
There was a problem hiding this comment.
I removed it 😅 let me add it again
|
@akshatnema added datatest-id again Why are tests failing? |
Check the logs, you will get your answer 🙂 |
… MigrateTypography
…website into MigrateTypography
|
@akshatnema , I looked into the logs and tried fixing the errors, but the tests are still failing. I think it's because of some linting issues. can you please have a look at it |
|
/rtm |
Desscription : Migration of typography to TS
related #2636