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

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

Merged
merged 7 commits into from
Feb 12, 2024

Conversation

Rishi556
Copy link
Contributor

@Rishi556 Rishi556 commented Nov 17, 2023

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

@mui-bot
Copy link

mui-bot commented Nov 17, 2023

Netlify deploy preview

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against f449700

@danilo-leal danilo-leal added package: lab Specific to @mui/lab component: masonry This is the name of the generic UI component, not the React module! labels Nov 23, 2023
@danilo-leal danilo-leal changed the title [lab][Masonry] Ability to sort Masonry elements from left to right [lab][Masonry] Ability to sort elements from left to right Nov 23, 2023
@Rishi556
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Rishi556 commented Jan 2, 2024

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:

child.style.order = nextOrder;

and here

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:

'&: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
Copy link
Member

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
Copy link
Contributor Author

Rishi556 commented Jan 3, 2024

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)

image

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

image

@DiegoAndai
Copy link
Member

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
Copy link
Contributor Author

Rishi556 commented Jan 8, 2024

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
Copy link
Member

@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 Rishi556 marked this pull request as ready for review January 10, 2024 15:24
@Rishi556
Copy link
Contributor Author

Let me know if that works out! 😊

It did, thanks!

return;
}
useEnhancedEffect(() => {
const handleResize = (masonryChildren) => {
Copy link
Member

@DiegoAndai DiegoAndai Jan 16, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Rishi556 commented Feb 9, 2024

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:

image

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.

image

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

@DiegoAndai
Copy link
Member

@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 🙌🏼

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

Thanks for the contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: masonry This is the name of the generic UI component, not the React module! package: lab Specific to @mui/lab
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Masonry] Ability to sort elements from left to right
5 participants