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

[Typography][joy] Convert Typography test to TypeScript #37165

Merged
merged 2 commits into from
May 12, 2023
Merged

[Typography][joy] Convert Typography test to TypeScript #37165

merged 2 commits into from
May 12, 2023

Conversation

DerTimonius
Copy link
Contributor

This PR will change the test for the Typography component to TypeScript with associated changes. The test is still running without issues.
See #37078

@mui-bot
Copy link

mui-bot commented May 4, 2023

Netlify deploy preview

https://deploy-preview-37165--material-ui.netlify.app/

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against 4ad6c67

@zannager zannager added component: Typography The React component. package: joy-ui Specific to @mui/joy labels May 5, 2023
@zannager zannager requested a review from hbjORbj May 5, 2023 09:39
import { expect } from 'chai';
import * as React from 'react';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this at the top of the file as it was

import { createRenderer, describeConformance, describeJoyColorInversion } from 'test/utils';
import Typography, { typographyClasses as classes } from '@mui/joy/Typography';
import { ThemeProvider } from '@mui/joy/styles';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not change the order of imports

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, forgot to disable the import sorter..

@@ -40,11 +40,13 @@ describe('<Typography />', () => {

['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'body1', 'body2', 'body3'].forEach((level) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'body1', 'body2', 'body3'].forEach((level) => {
(['h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'body1', 'body2', 'body3'] as const).forEach((level) => {

Comment on lines 48 to 49
const classToCheck: string | RegExp = classes[level as keyof typeof classes];
expect(container.firstChild).to.have.class(classToCheck);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the change. No error should be thrown if you address my comment above.

Comment on lines 106 to 107
expect((container.firstChild?.firstChild as HTMLElement)?.tagName).to.equal('SPAN');
expect((container.firstChild?.lastChild as HTMLElement)?.tagName).to.equal('B');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expect((container.firstChild?.firstChild as HTMLElement)?.tagName).to.equal('SPAN');
expect((container.firstChild?.lastChild as HTMLElement)?.tagName).to.equal('B');
const child = container.firstChild?.firstChild ?? null;
if (child === null) {
return;
}
expect((child as HTMLElement).tagName).to.equal('SPAN');
expect((child as HTMLElement).tagName).to.equal('B');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this change would be appropiate:
The first expect checks for the firstChild while the second checks for the lastChild. I'll be changing this so that I put the container.firstChild in a value and check against null

Copy link
Member

@hbjORbj hbjORbj May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I missed that. Yes I exactly meant what you've done :) Thanks for addressing the comment.

@hbjORbj hbjORbj changed the title [Typography][Joy]Convert Typography test to TypeScript [Typography][joy] Convert Typography test to TypeScript May 12, 2023
@hbjORbj hbjORbj merged commit 9b3dc8e into mui:master May 12, 2023
binh1298 pushed a commit to binh1298/material-ui that referenced this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Typography The React component. package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants