[DataGrid] Disable Hide button if there's only one visible column

[cherniavskii]

Closes #2718

Preview: https://deploy-preview-3607--material-ui-x.netlify.app/components/data-grid/

Hide.mov

[cherniavskii]

I'm struggling to make this test pass.

When clicking manually in the browser, disabled Hide button doesn't trigger click event, but in this test it does.

Hide button is a ButtonBase component rendered as a li element.
Since it's not a button, it doesn't have a disabled HTML attribute when disabled. Instead, it has aria-disabled + pointer-events: none.

I've noticed that imperative .click() called on disabled li ButtonBase does trigger click as well.
I believe same happens in tests.

Here's a minimal reproduction: https://codesandbox.io/s/disabled-buttonbase-click-c91hx?file=/src/App.js
Steps to reproduce:

1. Click Trigger .click() on ButtonBase imperatively
2. Nothing happens
3. Click Change ButtonBase component to "li"
4. Click Trigger .click() on ButtonBase imperatively again
5. onClick is called, even though ButtonBase is disabled

I'm not sure if we should consider this as a bug.

Any ideas how to make the test pass?
I've tried to use userEvent.touch instead of fireEvent.click, but it doesn't feel right.

[alexfauquette]

Since it seems the problem is from ButtonBase, the easiest way is probably to not test by click but only check that the button has aria-disabled

[cherniavskii]

@alexfauquette Thanks for looking into it!

the easiest way is probably to not test by click but only check that the button has aria-disabled

The downside I see here is that aria-disabled is an implementation detail, so if we change from li to button the test will fail.
Also, in the test we trust that if aria-disabled is present, onClick won't be triggered.
It would do the job for now though, so I think it's fine. Will add a comment explaining the decision.

[alexfauquette]

Adding a comment to explain the decision is a good idea 👍

[m4theushw]

Do you know if the test only fails in JSDOM? It could be some limitation of JSDOM. Tests running with yarn test:karma use a real browser so it should behave in the same way as when manually clicking. Another thing you could do is to check if the button is disabled inside the onClick handler.

[cherniavskii]

Do you know if the test only fails in JSDOM?

It fails in test:karma

Another thing you could do is to check if the button is disabled inside the onClick handler

Yeah, that would allow to keep fireEvent.click in the test

[m4theushw]

It gives a more friendly message when it fails.

Suggested change

	expect(hideButton.getAttribute('aria-disabled')).to.equal('true');
	expect(hideButton).to.have.attribute('aria-disabled', 'true');

[cherniavskii]

Huh, turns out the message is not friendly at all when it fails:

TypeError: Converting circular structure to JSON
            --> starting at object with constructor 'HTMLLIElement'
            |     property '__reactFiber$x0ublx5qav' -> object with constructor 'FiberNode'
            --- property 'stateNode' closes the circle
            at JSON.stringify (<anonymous>)
            at processAssertionError (node_modules/karma-mocha/lib/adapter.js:59:41)
            at Runner.<anonymous> (node_modules/karma-mocha/lib/adapter.js:164:28)

I tried another test in the codebase that uses this notation and made it fail - got the same output.
For example here:
https://github.com/mui-org/material-ui-x/blob/7cc6c3c68e139836fec65505254a311534f1e929/packages/grid/x-data-grid/src/tests/selection.DataGrid.test.tsx#L327

If assertion fails - you'll see TypeError: Converting circular structure to JSON

Looks like an issue with mocha, I'll look into it later

[m4theushw]

LGTM