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

[material-icons] Testing #22622

Closed
1 task done
at-webdev opened this issue Sep 16, 2020 · 6 comments · Fixed by #22634
Closed
1 task done

[material-icons] Testing #22622

at-webdev opened this issue Sep 16, 2020 · 6 comments · Fixed by #22634
Labels
good first issue Great for first contributions. Enable to learn the contribution process. package: icons Specific to @mui/icons test

Comments

@at-webdev
Copy link

at-webdev commented Sep 16, 2020

I am using React Testing Library, Is it possible to test in specific Material UI icon as ArrowLeft / ArrowRight instead of .MuiSvgIcon-root?

App component:

return {open ? <ArrowLeft/> :<ArrowRight/>}

RTL Testing : Below tests are passing but it doesn't check in specific ArrowLeft or ArrowRight icon.

describes("MockTest",()=>{

it("renders Left arrow",()=>{
const {container} = renders(<App open={true}/>);
expect(container.querySelector(".MuiSvgIcon-root").toBeTruthy();
});

it("renders Right arrow",()=>{
const {container} = renders(<App open={false}/>);
expect(container.querySelector(".MuiSvgIcon-root").toBeTruthy();
});

});
  • I have searched the issues of this repository and believe that this is not a duplicate.
@at-webdev at-webdev added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 16, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 16, 2020

@at-webdev For internal test purposes, we leverage:

https://github.com/mui-org/material-ui/blob/8452e7195ffa2e9f823ea9f44681739b8e33b797/packages/material-ui/src/utils/createSvgIcon.js#L9

However, this attribute is removed once the package is published on npm (to save bundle size and not leak test internals) with:

https://github.com/mui-org/material-ui/blob/8452e7195ffa2e9f823ea9f44681739b8e33b797/babel.config.js#L38

Now, considering we frequently need this in our tests, I would be in favor of making it public with:

diff --git a/packages/material-ui/src/utils/createSvgIcon.js b/packages/material-ui/src/utils/createSvgIcon.js
index 2d146ab87d..5f53ccca28 100644
--- a/packages/material-ui/src/utils/createSvgIcon.js
+++ b/packages/material-ui/src/utils/createSvgIcon.js
@@ -6,7 +6,7 @@ import SvgIcon from '../SvgIcon';
  */
 export default function createSvgIcon(path, displayName) {
   const Component = (props, ref) => (
-    <SvgIcon data-mui-test={`${displayName}Icon`} ref={ref} {...props}>
+    <SvgIcon data-testid={`${displayName}Icon`} ref={ref} {...props}>
       {path}
     </SvgIcon>
   );

@eps1lon What do you think?

@oliviertassinari oliviertassinari changed the title RTL Testing Material UI Icon [material-icons] Testing Sep 16, 2020
@oliviertassinari oliviertassinari added package: icons Specific to @mui/icons test and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 16, 2020
@eps1lon
Copy link
Member

eps1lon commented Sep 16, 2020

I think icons are the one case where we could make an exception since they have reasonable user impact and no simple alternative. I just hope we don't have to argue why we ship them for icons but not component X.

I'd consider the bundle size implications negligible. If you've got many icons on your page then either: compression will flatten the curve or you should look at <use>.

And as a little bit of anecdotal cherry on top: facebook ships them in their SSR payload as well and twitter renders them client side.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Sep 16, 2020
@jaebradley
Copy link
Contributor

@oliviertassinari do you mind if I try implementing this?

And would you still want to keep the data-mui-test attribute for posterity (even if it gets removed before publication) or would you prefer if it was swapped with data-test-id?

@oliviertassinari
Copy link
Member

@jaebradley in the past, we have discussed replacing as many data-mui-test attributes with built-in accessibility features.

Assuming we will keep pushing in this direction, I think that we can rename the prop in SvgIcon and update all the tests that depends on it.

@eps1lon
Copy link
Member

eps1lon commented Sep 17, 2020

I wanted to have a look at existing data-mui-test usage anyway. Hopefully we don't even use them anymore at which point we might as well remove them. Otherwise people might get confused if they look at the source code (not that I wouldn't prefer this to be impossible but that kind of doesn't work with open source 😄 ).

@kylegoetz
Copy link

kylegoetz commented Nov 10, 2021

Shouldn't people be giving their semantic icons titleAccess prop and then querying for that? MUI's own documents say you should use titleAccess for accessibility purposes.

I realize this issue is closed, but here's the language in question:

Semantic SVG icons
You should include the titleAccess prop with a meaningful value. The role="img" attribute and the <title> element are added so that your icons are correctly accessible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Great for first contributions. Enable to learn the contribution process. package: icons Specific to @mui/icons test
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants