[Portal] Second iteration on the component

[oliviertassinari]

This pull-request revamp the implementation of the Portal and the Modal component.
Those two components are core and used by many more components: Dialog, Drawer, Snackbar, Popover, Menu, @select.
This effort might also fix some issues with the Select component, to investigate.

• Exposes and document our Portal component
• Exposes and document our Modal component
• Remove piece of logic for IE8
• Exposes Backdrop component

Closes #9528 as it's documented
Closes #9252 as we take the owner document into account
Closes #9248 as the new portal component behave correctly during the reconciliation
Closes #9207 as we take the owner document into account
Fix one part of #9474

Breaking changes

Some properties have been renamed:

<Dialog
- ignoreBackdropClick
- ignoreEscapeKeyUp
+ disableBackdropClick
+ disableEscapeKeyDown

<Modal
- show
- disableBackdrop
- ignoreBackdropClick
- ignoreEscapeKeyUp
- modalManager
+ open
+ hideBackdrop
+ disableBackdropClick
+ disableEscapeKeyDown
+ manager

The zIndex object has been updated to match the usage.

  const zIndex = {
-  mobileStepper: 900,
-  menu: 1000,
+  mobileStepper: 1000,
   appBar: 1100,
-  drawerOverlay: 1200,
-  navDrawer: 1300,
-  dialogOverlay: 1400,
-  dialog: 1500,
-  layer: 2000,
-  popover: 2100,
-  snackbar: 2900,
-  tooltip: 3000,
+  drawer: 1200,
+  modal: 1300,
+  snackbar: 1400,
+  tooltip: 1500,
  };

[gregnb]

    <Portal container={this.container}>
      <Typography>But I actually render here!</Typography>
    </Portal>

Very excited to see the ability to specify containers! Would love the ability to tap into that from the Popover component

[oliviertassinari]

Would love the ability to tap into that from the Popover component

@gregnb You can with the "inheritance" of the properties.

[gregnb]

@oliviertassinari that's great! thanks for working on this

[jedwards1211]

@oliviertassinari I don't think this is necessarily appropriate for Drawer. For one, who's to say we don't want to use a Drawer within a certain component rather than at root level?

For another, you're assuming we want the backdrop to show when the drawer is open, but that's not the case when we're using the drawer for a responsive Sidebar, which shouldn't interfere with the content when it's open on a large enough display.

[oliviertassinari]

@jedwards1211 I'm not sure to follow. What's your conclusion? The Modal is only used by the temporary type of the Drawer.

[jedwards1211]

Sorry yeah, I wasn't used to the structure of the new docs, I discovered the different types in the demos section.

[corytheboyd]

@oliviertassinari Awesome! Can't wait to see this released, it's all we're waiting for to overhaul the UI on a project!

[oliviertassinari]

@corytheboyd We release every weekend. You can start next week :).

[pandaiolo]

@oliviertassinari this PR removes displayName from HOC like withStyle(Component) (https://github.com/mui-org/material-ui/pull/9613/files#diff-579da44d8bf8da76dcc331a87958265bL317)

I am using it to identify components selected in my Enzyme tests, which results broken for all components wrapped in this HOC. I could refactor to use unwrapped components only, but I was wondering if this is on purpose? Any reason for this removal? (ie should I do the same in my own HOC :-) )

(rel https://reactjs.org/docs/higher-order-components.html#convention-wrap-the-display-name-for-easy-debugging)

Thanks!

[oliviertassinari]

Convention: Wrap the Display Name for Easy Debugging

@pandaiolo The motivation is around developer experience. Let's take an example on the documentation website:

How is displaying WithStyles(Connect(AppFrame)) easing the debugging? When instead, you can avoid duplication with a shorter wording WithStyles? cc @rosskevin how raised the issue. Pointing our that compound components are superior part for this reason.

[oliviertassinari]

This React documentation section was introduced by @acdlite in reactjs/react.dev@e3d37fd. Maybe he can 💡 us.

[oliviertassinari]

As well as @istarkov maintaining recompose, what do you think of this displayName logic change?

[oliviertassinari]

Same story with the React warnings? What do you prefer between?
1.

Warning: React does not recognize the enterrDelay prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase enterrdelay instead. If you accidentally passed it from a parent component, remove it from the DOM element.
in button (at ButtonBase.js:240)
in ButtonBase (at withStyles.js:293)
in WithStyles (at Button.js:191)
in Button (at withStyles.js:293)
in WithStyles (at FloatingActionButtonZoom.js:119)

Warning: React does not recognize the enterrDelay prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase enterrdelay instead. If you accidentally passed it from a parent component, remove it from the DOM element.
in button (at ButtonBase.js:240)
in ButtonBase (at withStyles.js:293)
in WithStyles(ButtonBase) (at Button.js:191)
in Button (at withStyles.js:293)
in WithStyles(Button) (at FloatingActionButtonZoom.js:119)

[pandaiolo]

Ok I see, I agree it is more readable in the DOM and in the debugging output!

However, I believe the displayName also has an identification purpose (for example in my use case in Enzyme selectors), which is defeated when the wrapping is removed, because it makes all wrapped components look alike.

Also, is wrapped displayName that an issue? IMHO the visual improvement is not worth the loss of identification. Not sure if this is only my personal use case though, or if I should not use displayName for that!

[oliviertassinari]

@pandaiolo I don't have a strong opinion on this change. Just not sure the value, we get out of it, is obvious.

I could refactor to use unwrapped components only

No need for that, I have been updating the tests internally:

-assert.strictEqual(wrapper.name(), 'withStyles(Paper)');
+assert.strictEqual(wrapper.type(), Paper);

[pandaiolo]

No need for that, I have been updating the tests internally:

I also use withStyle to export styled components in my app. Do you know how could I dynamically get a component name that I can then use in Enzyme selector? For example:

function testUtil(wrapper, component, ...) {
  const selector = component.type.displayName
  wrapper.find(component)
  // do some other things
}

The above code example is the one that is broken by this PR in my case

[oliviertassinari]

Do you know how could I dynamically get a component name that I can then use in Enzyme selector?

@pandaiolo Why don't you .dive() instead or us our test helpers?

[oliviertassinari]

Also, you can use find with the component as argument. It's less brittle than using the displayName.

[pandaiolo]

Sometimes find() is too strict. Here it is a flexible util that checks only the type and the props, whatever other props and children the target has. For example:

// wrapper: 
// <WrappingStuff>
//    <WithChild prop="val" otherProp="other">{child}</withChild>
// </WrappingStuff />
itHas(wrapper, <WithChild prop="val" />)
// true

I guess this would work: itHas(wrapper, 'WithChild', {prop: 'val'}) but I prefer the other one :-)

[pandaiolo]

Or using MUI test-util unwrap?

const { displayName } = component.Naked ? unwrap(component) : component
wrapper.find(displayName)

Edit: this only works for components, not elements like in my example... so there is no way to know on a HOC element what is the underlying component.

[pandaiolo]

Ok I did not yet find a way to resolve the above (minor) issue, but in the meantime just realized the true extent of the trouble in Enzyme debugging:

const wrapper = shallow(<TestedComponent />)
wrapper.debug()
// Readable element tree 

Then, whenever tests fail, there is a very useful wrapper.debug() util in Enzyme that shows shallow elements failing. However, now all the DOM just show <WithStyle> elements, because almost all of them are styled!

Any help to workaround this one very much appreciated!

Edit: Real life example:

<Aux>
      <WithStyles dense={true} className="small" color="primary" disabled={false} onClick={[Function: value]}>
        <pure(AddCircle) />
        xxxx
      </WithStyles>
      <WithStyles open={true} classes={{...}}>
        <WithStyles>
          xxxx
        </WithStyles>
        <WithStyles classes={{...}}>
          <Connect(WithTheme) includeXxxx={true} xxxId={51} onChange={[Function]} />
          <h2>
            Xxxx:
            <span className="error">
               (Warning: Xxx xx xx)
            </span>
          </h2>
          <XxxInput fullWidth={true} name="xxx" value={10} onChange={[Function]} />
        </WithStyles>
        <WithStyles>
          <WithStyles onClick={[Function: value]}>
            Cancel
          </WithStyles>
          ,
          <WithTheme dialogTitle="Please confirm" text="Are you sure you want to xxxx?">
            <WithStyles raised={true} color="primary" onClick={[Function: value]} disabled={false}>
              Save
            </WithStyles>
          </WithTheme>
        </WithStyles>
      </WithStyles>
    </Aux>

See how WithStyle and WithTheme break displayName where recompose pure() and redux connect() forward the underlying element name?

[oliviertassinari]

@pandaiolo I feel like we should revert and stick to the community convention (Unrelated to your testing issue).