Media match breakpoint index

[atanasster]

issue: #1191

useBreakpointIndex was returning the number of breakpoints matched, while the default value (defaultIndex) and the name of the hook were implying that the returned values is an index into the array of breakpoints.

The tests were also not correct (adjusted to the wrong implementation) and had to be changed - unfortunately now this can break existing code that would have been relying on the previous code. Usually in only happens if you provided more values than breakpoints are available

For example this used to work

const Component = (props) => {
  //4 values provided - previously it would return 'd' - the last value
    value = useResponsiveValue(['a', 'b', 'c', 'd'])
    return null
  }
<ThemeProvider
      theme={{ //3 breakpoints
        breakpoints: ['30em', '45em', '55em'],
      }}>
      <Component />
</ThemeProvider>

Now an exception will be generated if more values are used than the number of available breakpoints

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/systemui/theme-ui/49159k1yo
✅ Preview: https://theme-ui-git-media-match-breakpoint-index.systemui.vercel.app

[Deployment for cf19ba6 canceled]

[lachlanjc]

Looks good! Is this breaking? Do we need to update any docs?

[atanasster]

Looks good! Is this breaking? Do we need to update any docs?

its is breaking for users who relying on the buggy implementation before - hopefully not that many. IN normal use, it is not breaking, butin any case I marked it now as BREAKING in CHANGELOG

[hasparus]

Wait, I don't get it.

Given 3 breakpoints, there are 4 possible responsive values, right?

value-0 breakpoint-A value-1 breakpoint-B value-2 breakpoint-C value-3  

How does this change affect usage of useBreakpointIndex?

[lachlanjc]

Is this bug still a problem/do we want this PR now?

[lachlanjc]

Closing as stale, but we can totally re-open!

[korac]

hi there 👋
I would like to re-open a discussion on this one.

I believe there's a mismatch in terminology here and consequently, in the implementation.

As stated in the first comment, useBreakpointIndex:

• returns the number of breakpoints matched.
• receives an index of a specific breakpoint (defaultIndex).

In the implementation:

• the hook uses the defaultIndex (which is an index in breakpoints array) to initialize value (local state).
• but getIndex function filters the number matched breakpoints (or better to say, matches the index of a responsive value) - which allows the value to become larger than the index in breakpoints array.

Consider this example:

const breakpoints = ['640px', '1240px'];

const bpIndex = useBreakpointIndex(); // Can be one of: `0 | 1 | 2` because the hook returns the *number* of matched breakpoints

const bpIndex2 = useBreakpointIndex({ defaultIndex : 2 }); // throws an error because defaultIndex, in this implementation, tracks the index of the breakpoint (not their number).

Basically, what useBreakpointIndex receives as an argument and returns are, currently, two distinct notions.

Quick fix

If believe an easy fix for this is simply allowing defaultIndex to be the same length as breakpoints array.

// /match-media/src/index.ts

if (typeof defaultIndex !== 'number') {
  // ...
} else if (defaultIndex < 0 || defaultIndex > breakpoints.length) { // Not: breakpoints.length - 1
  // ...
}