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

[ClickAwayListener] Fix children and onClickAway types #24565

Merged
merged 12 commits into from
Jan 27, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jan 23, 2021

Based on #24564 (diff)

children allowed a nullable ReactNode when we actually treat it as a non-nullable ReactElement.
onClickAway would allow treating the event as a React event when we only pass it a native event.

Test plan

  • check declarations for too new TypeScript syntax (e.g. template literal types)

@mui-pr-bot
Copy link

mui-pr-bot commented Jan 23, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 569dcf4

@eps1lon eps1lon force-pushed the feat/ClickAwayListener/typescript branch from 36490d9 to 6f10c71 Compare January 23, 2021 17:12
@eps1lon eps1lon marked this pull request as ready for review January 25, 2021 08:00
@eps1lon eps1lon marked this pull request as draft January 25, 2021 08:05
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jan 25, 2021
if (
!activatedRef.current ||
!nodeRef.current ||
('clientX' in event && clickedRootScrollbar(event, doc))
Copy link
Member Author

Choose a reason for hiding this comment

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

We used to access event.clientX on touch events which don't implement that property. So we would always end up evaluating doc.documentElement.clientWidth < undefined || doc.documentElement.clientHeight < undefined. This always evaluates to false so we can bail out early by checking if we have a MouseEvent.

In the end we're forcing less layout on touch events (no access to element.clientWidth) but check property existence more often. Seems like a good tradeoff considering forcing layout is much more expensive than checking property existence and touch events are usually dispatched on weaker devices e.g. smartphones.

@eps1lon eps1lon force-pushed the feat/ClickAwayListener/typescript branch from 2980efa to 569dcf4 Compare January 26, 2021 12:55
@eps1lon eps1lon marked this pull request as ready for review January 26, 2021 15:11
@eps1lon eps1lon merged commit c229cbe into mui:next Jan 27, 2021
@eps1lon eps1lon deleted the feat/ClickAwayListener/typescript branch January 27, 2021 11:21
xs9627 added a commit to xs9627/material-ui that referenced this pull request Jan 27, 2021
* next: (34 commits)
  [Tab] Migrate to emotion (mui#24651)
  [TextField] Migrate Input component to emotion (mui#24638)
  [ImageList] Migrate ImageListItemBar to emotion (mui#24632)
  [CircularProgress] Migrate to emotion (mui#24622)
  [DataTable] Add example in docs for data table (mui#24428)
  [Card] Migrate CardActionArea to emotion (mui#24636)
  [core] Fix `next` using stale pages (mui#24635)
  [List] Migrate ListItemIcon to emotion (mui#24630)
  [ClickAwayListener] Fix `children` and `onClickAway` types (mui#24565)
  [docs] Include in docs directive to silence `eslint` erroneous warning (mui#24644)
  [Fab] Migrate to emotion (mui#24618)
  [TextField] Migrate FilledInput to emotion (mui#24634)
  [Card] Migrate CardHeader to emotion (mui#24626)
  [Card] Migrate CardMedia to emotion (mui#24625)
  [ImageList] Migrate ImageListItem to emotion (mui#24619)
  [website] Add vision block (mui#24603)
  [docs] Add sorting section (mui#24637)
  [TextField] Prepare removal of labelWidth prop (mui#24595)
  [Dialog] Migrate DialogTitle to emotion (mui#24623)
  [ImageList] Migrate to emotion (mui#24615)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants