-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[core] Begin removing IE 11-related code #41709
[core] Begin removing IE 11-related code #41709
Conversation
Netlify deploy previewhttps://deploy-preview-41709--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
fe72fc3
to
3663cca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
Could you please remove the no IE 11 support
comment statement from the .eslintrc.js
file at https://github.com/mui/material-ui/blob/next/.eslintrc.js#L167. We can handle this in the current PR.
Also, I've noticed that you haven't removed all instances of IE11 from the JS/TS code (not CSS), but I believe we can address that in separate PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iammminzzy Apologies for not mentioning this in the previous review, but could you also address the following tasks in this PR:
-
Replace
stableSort
in the following locations:- Joy UI Table demo: https://github.com/mui/material-ui/blob/next/docs/data/joy/components/table/TableSortAndSelection.tsx#L101
- Order dashboard template in Joy UI: https://github.com/mui/material-ui/blob/next/docs/data/joy/getting-started/templates/order-dashboard/components/OrderTable.tsx#L249
- Enhanced Table demo: https://github.com/mui/material-ui/blob/next/docs/data/material/components/table/EnhancedTable.tsx#L95
- Folder Table showcase: https://github.com/mui/material-ui/blob/next/docs/src/components/showcase/FolderTable.tsx
-
Remove the comment text
The property isn't supported by IE11
from:- Joy UI Avatar component: https://github.com/mui/material-ui/blob/next/packages/mui-joy/src/Avatar/Avatar.tsx#L79
-
Remove the comment text
⚠️ sticky is not supported by IE11.
from:- Material UI AppBar component: https://github.com/mui/material-ui/blob/next/packages/mui-material/src/AppBar/AppBar.js#L73
-
Remove the comment text
The property isn't supported by IE11.
from:- Material UI Avatar component: https://github.com/mui/material-ui/blob/next/packages/mui-material/src/Avatar/Avatar.js#L89
-
Remove the comment from:
- Material UI CardMedia component: https://github.com/mui/material-ui/blob/next/packages/mui-material/src/CardMedia/CardMedia.js#L46
-
Replace the usage of
isNaN
withNumber.isNaN
at:- Material UI Tabs component: https://github.com/mui/material-ui/blob/next/packages/mui-material/src/Tabs/Tabs.js#L423
I believe these tasks can be addressed in this PR. The remaining tasks can be handled separately with thorough testing.
Per the original PR description, it’s not as simple as that. If you actually analyse the code, you’ll see that what gets stored is always a number, never a |
1c03796
to
382f3c3
Compare
You're correct. It seems that the value never becomes --- a/packages/mui-material/src/Tabs/Tabs.js
+++ b/packages/mui-material/src/Tabs/Tabs.js
@@ -420,9 +420,10 @@ const Tabs = React.forwardRef(function Tabs(inProps, ref) {
[size]: tabMeta ? tabMeta[size] : 0,
};
- // IE11 support, replace with Number.isNaN
- // eslint-disable-next-line no-restricted-globals
- if (isNaN(indicatorStyle[startIndicator]) || isNaN(indicatorStyle[size])) {
+ if (
+ typeof indicatorStyle[startIndicator] !== 'number' ||
+ typeof indicatorStyle[size] !== 'number'
+ ) {
setIndicatorStyle(newIndicatorStyle);
} else {
const dStart = Math.abs(indicatorStyle[startIndicator] - newIndicatorStyle[startIndicator]); Looking back at the history, I'm not sure why it was added in #8831. It appears that it never actually became the global I believe using a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. We just need approval from @mui/code-infra before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see nothing out of the ordinary, but let's wait for @michaldudak 's review before merging.
Some comments that are out of scope for this PR:
- Are we polyfilling
.toSorted
already? To dodata.toStorted()
instead of[...data].sort()
. - Intuitively I'd expect an array sort in a render to be memoized in 99% of the cases.
We did consider that but TypeScript seems to be configured with |
This reverts commit 382f3c3.
2e098c4
to
974c9ec
Compare
Hey @iammminzzy! Thanks for working on this. We should add any breaking changes to the migration guide: https://github.com/iammminzzy/material-ui/blob/98494bd51919f4b299ffa864c0097f3bea134d75/docs/data/material/migration/migration-v5/migration-v5.md May I ask you to add a section summarizing the changes on this PR and #41611 |
See #14420.
Per #41611 (comment) I have cherry-picked code changes from #41611 into a separate PR: