[lab][Masonry] Ability to sort elements from left to right

[Rishi556]

• I have followed (at least) the PR section of the contributing guide.
• Create Tests
• Fix eslint issue

Resolves #36859. Allows sorting masonry from left to right with the sequential prop.

[mui-bot]

Netlify deploy preview

• docs/data/material/components/masonry/masonry.md

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against f449700

[Rishi556]

I'm stuck on the tests for this, if anyone has advice I'd appreciate it. For some reason, the computed style always has order as "0" in the tests. I did notice that none of the other tests check for order other than SSR. Would it be better to check for the height of the whole Masonry element than checking order?

[DiegoAndai]

Hey @Rishi556! Thanks for working on this

For some reason, the computed style always has order as "0" in the tests

I assume you mean height is always 0? If I recall correctly, the height is 0 until the elements are placed. So maybe in your tests, you must wait until elements are placed. Let me know if that works.

[Rishi556]

Hey @Rishi556! Thanks for working on this

For some reason, the computed style always has order as "0" in the tests

I assume you mean height is always 0? If I recall correctly, the height is 0 until the elements are placed. So maybe in your tests, you must wait until elements are placed. Let me know if that works.

Sorry about the delayed response, holidays and such(Happy New Years)! I do mean the order(guess I force pushed away my initial tests using order), but you can see it being set here:

material-ui/packages/mui-lab/src/Masonry/Masonry.js

Line 267 in e03cb44

child.style.order = nextOrder;

and here

material-ui/packages/mui-lab/src/Masonry/Masonry.js

Line 277 in e03cb44

child.style.order = order;

It basically seems to control which column the masonry element is in. The only test case that seems to use is it the server side rendering one:

material-ui/packages/mui-lab/src/Masonry/Masonry.test.js

Lines 267 to 270 in e03cb44

	'&:nth-of-type(4n+1)': { order: 1 },
	'&:nth-of-type(4n+2)': { order: 2 },
	'&:nth-of-type(4n+3)': { order: 3 },
	'&:nth-of-type(4n+0)': { order: 4 },

I'll look into seeing how to tell when elements are placed to grab the order and see if that helps the test case(do you have any examples of it from other tests), thanks!

[DiegoAndai]

Happy New Year to you as well, @Rishi556!

I wonder if this is a JSDOM limitation 🤔 have you tried running the tests with karma? yarn test:karma should run them.

To target the Masonry component specifically, you can modify your local karma.tests.js to:

import '@mui-internal/test-utils/init';
import '@mui-internal/test-utils/setupKarma';

const labUnitContext = require.context(
  '../packages/mui-lab/src/Masonry',
  true,
  /\.test\.(js|ts|tsx)$/,
);
labUnitContext.keys().forEach(labUnitContext);

Otherwise yarn test:karma will run the entire suite.

[Rishi556]

Hey @DiegoAndai. I am running the tests via Karma. Here's an example of what i mean by the order:

it('should place children in sequential order', function test() {
      if (/jsdom/.test(window.navigator.userAgent)) {
        // only run on browser
        this.skip();
      }

      const { getByTestId } = render(
        <Masonry columns={2} spacing={1} sequential>
          <div style={{ height: `20px` }} data-testid="child1" />
          <div style={{ height: `10px` }} data-testid="child2" />
          <div style={{ height: `10px` }} data-testid="child3" />
        </Masonry>,
      );

      const child1 = getByTestId('child1');
      const child2 = getByTestId('child2');
      const child3 = getByTestId('child3');
      console.log(
        'c1',
        window.getComputedStyle(child1).order,
        'c2',
        window.getComputedStyle(child2).order,
        'c3',
        window.getComputedStyle(child3).order,
      );
      expect(window.getComputedStyle(child1).order).to.equal(`1`);
      expect(window.getComputedStyle(child2).order).to.equal(`2`);
      expect(window.getComputedStyle(child3).order).to.equal(`1`);
    });

We'd expect the children's order to be 1,2, and 1 again(and can be verified in this code sandbox: https://codesandbox.io/p/sandbox/optimistic-voice-s2ghtq?file=%2Fsrc%2FDemo.tsx)

But that's not what our test case outputs, instead it's all 0's.

[DiegoAndai]

The Masonry doesn't place its elements on the initial render (see this issue). The way it works is that it renders the elements initially (with order: 0), and then it calculates the correct order. That's what I think might be happening here. You need to wait for the placement and obtain the order values afterward. Let me know if that helps.

[Rishi556]

I attempted to use forceUpdate() in order to get it to render again, but it doesn't look like that's the right one to use. Do you know how I should go about waiting?

[DiegoAndai]

@Rishi556 I haven't used it, but maybe you could try the Testing Library's waitFor. Another option would be to await a timeout (created with setTimeout).

For both, you'll need to turn the test into an async function and be careful about the test not timing out (although I think the ordering should take around 1 ms, so it shouldn't time out).

Let me know if that works out! 😊

[Rishi556]

Let me know if that works out! 😊

It did, thanks!

[DiegoAndai]

Is it necessary to move handleResize inside the effect? I would rather keep it out, if possible.

[Rishi556]

I had to move it inside because of a eslint rule. I'll see if I can get it working without moving it in and not having to wrap handleResize in a useCallback.

[Rishi556]

Should be good now, let me know what you think.

[DiegoAndai]

Hey @Rishi556. First of all, I'm sorry for the delay, it has been a busy week.

It looks good overall. My only question would be, is the useCallback necessary? If it's necessary so the components reorders when the sequential prop value changes, I think we could achieve the same by adding sequential as a dependency to the useEnhancedEffect that calls handleResize. Or is it necessary for another reason?

I'm asking because I would like to add this functionality with as minimum change to the component as possible.

[Rishi556]

Thanks for the response, no worries on timing!

I had initially just added sequential to the useEnhancedEffect dependency array and that caused eslint issues which this resolved:

Adding handleResize as well to the dependency array causes it to need to be wrapped in a useCallback or moved within useEnhancedEffect which you requested to leave it outside of it.

(A lot of this stems from some of my inexperience with React, so I'm always eager to hear of ways to improve!)

[DiegoAndai]

@Rishi556 This is weird; I tested, and I don't know why handleResize was not throwing the exhaustive-deps warn before. It seems to me that it should've 😅

Anyway, I think your implementation is correct and seems to be working. I'll review again on Monday with a fresher brain to see if I can spot any potential regressions. If it all seems fine, then I think we could merge this 🙌🏼

[DiegoAndai]

LGTM 🎉

Thanks for the contribution