[@mantine/core] Separate handle breakpoints override in mergeTheme

[7iomka]

Related issues:
#4050

Instead of merge breakpoints, use breakpoints from override if provided (with an additional check for required keys).

[cyantree]

Another approach would be:

1. Merge the breakpoints
2. Sort the resulting object by breakpoint pixel value as IMHO this is the expected order

Then you wouldn't need to specify all default breakpoints when overriding them.

With your approach it's arguable that this is a breaking change as partial breakpoint overrides wouldn't work anymore and have to be adjusted to work again.
Or am I missing something?

[7iomka]

@cyantree Yes, you are right, thanks!
I used your suggestion, please see the latest commit.

[cyantree]

Maybe also add a test for this in merge-theme.test.ts.

[7iomka]

@cyantree I'm not good in tests.
I tried a simple test but I get errors when I try to run
Tell me what I did wrong.
or how to write a test to pass?)
because according to the code it should pass.

[cyantree]

It failed because there's an error in your change implementation. So your test caught this. 👍

Here's the fixed code (you need to merge with acc):

    if (key === 'breakpoints' && themeOverride.breakpoints) {
      const mergedBreakpoints = { ...currentTheme.breakpoints, ...themeOverride.breakpoints };

      return {
        ...acc,
        breakpoints: Object.fromEntries(
          Object.entries(mergedBreakpoints).sort(
            (a, b) => getBreakpointValue(a[1]) - getBreakpointValue(b[1])
          )
        ),
      };
    }

If you want to cover more cases test merging with multiple units and also define your custom breakpoints in descending order. Maybe also test what happens with your overriding an existing breakpoint.

Here's an updated test for this:

    expect(
      Object.entries(mergeTheme(themeBase, { breakpoints: { xxl: '999em', min: '1', xs: '10rem' } }).breakpoints)
    ).toStrictEqual(Object.entries({ min: '1', ...{ ...themeBase.breakpoints, xs: '10rem' }, xxl: '999em' }));

I've also introduced Object.entries() as direct object checking doesn't regard the key order as normally this isn't relevant and therefore might make writing tests more harder than necessary:

// This passes but shouldn't in our case
expect({ a: 1, b: 1 }).toStrictEqual({ b: 1, a: 1 });

// This correctly fails
expect(Object.entries({ a: 1, b: 1 })).toStrictEqual(Object.entries({ b: 1, a: 1 }));

[7iomka]

@cyantree Yes, thank you very much for correcting the error and the tests!

[rtivital]

Thanks!