Support ActionMenu having null children

[y-lohse]

In Drive we have an ActionMenu but not all items are visible at all time — if one of the items should be hidden, it returns null instead, as is common in React. This was breaking ActionMenu because it was checking null.type.

I also added an example for the right prop of ActionMenuItem so that we have visual regression tests for it.

[ptbrowne]

Looking good, but can you add a test for the null case ?

[Crash--]

In Drive we have an ActionMenu but not all items are visible at all time — if one of the items should be hidden, it returns null instead, as is common in React

We're thinking about that and it's not impossible that instead of hiding features, we'll simply "disabled them" by displaying them with a special style and removing the onclick action.

[y-lohse]

We're thinking about that and it's not impossible that instead of hiding features, we'll simply "disabled them" by displaying them with a special style and removing the onclick action.

Some will likely stay fully hidden, for example those that we want either in the topbar or in the menu depending on the context.

Looking good, but can you add a test for the null case ?

Yup, done !

[ptbrowne]

This is a null string here ?

I think you want { null }.

[y-lohse]

🤦‍♂️thanks

[ptbrowne]

Was the testing passing with the "null" string ? If yes, this could be a problem.

[ptbrowne]

Why is this needed ?

[Crash--]

We have issue for testing component using Tooltip from MUI. See mui/material-ui#15726 (comment)

jsdom-16 fix the issue, but we need to use https://www.npmjs.com/package/jest-environment-jsdom-sixteen. I tried few months ago but still the issue. Maybe it's fine now.

[ptbrowne]

AMHA this should not be in this test file and should be called with a name that reviewers can understand.

Tooltip/testing.js or testing.js, something like that:

const fixTooltipTesting = () => {
  let createRangeBackup
  beforeAll(() => {
    createRangeBackup = global.document.createRange
    global.document.createRange = jest.fn(() => ({
      setStart: () => {},
      setEnd: () => {},
      commonAncestorContainer: {
        nodeName: 'BODY',
        ownerDocument: document
      }
    }))

  afterAll(() => {
    global.document.createRange = createRangeBackup
  })
}

This way we do not need a comment, it's clearer for the reviewer.

[Crash--]

and we can import it from other libs 👍

[ptbrowne]

👍

[cozy-bot]

🎉 This PR is included in version 35.11.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀