stimulus-use Introduce `useBreakpoints` mixin

Introduce `useBreakpoints` mixin

Open marcoroth opened this issue 3 years ago • 8 comments

First off: This PR is a proposal and I'm open for any feedback/improvements.

This PR introduces the useBreakpoints mixin. This mixin adds new behaviors to your Stimulus controller when the breakpoint changes or when a specific breakpoint is active.

I included a ton of options so you can really get a lot out of this mixin and customize the heck out of it. Check out the docs for all options.

I'm happy to add tests as soon as we nailed down the usage of this mixin 😊

Basic Usage

// breakpoints_controller.js

import { Controller } from 'stimulus'
import { useBreakpoints } from 'stimulus-use'

export default class extends Controller {
  connect() {
    useBreakpoints(this)
  }

  breakpointChanged({ breakpoint, width }) {
    // gets called whenever the breakpoint changes
  }
  
  breakpointSM({ width }) {
    // gets called whenever the current breakpoint changes to sm
  }

  breakpointXL({ width }) {
    // gets called whenever the current breakpoint changes to xl
  }
}

Default Breakpoints

This mixin ships with some default breakpoints so you don't need to configure the breakpoints on your own. There are currently defaults for Bootstrap and Tailwind in

https://github.com/stimulus-use/stimulus-use/blob/961b9278d29b439f8a92f161e5c03e96c83c10fb/src/use-breakpoints/breakpoints.ts#L1-L10

You can include them like this:

useBreakpoints(this, { breakpoints: Breakpoints.bootstrap })

or define them on your own:

useBreakpoints(this, {
  breakpoints: {
    'xs': 0,
    'sm': 576,
    'md': 768,
    'lg': 992,
    'xl': 1200,
    'xxl': 1400
  }
})

The power of events

I think the events in the mixin are really useful. This allows you to do stuff like this:

<body data-controller="breakpoints">
  <nav
    data-controller="navigation"
    data-action="breakpoint:changed@window->navigation#adapt">
  </nav>

  <aside
    data-controller="sidebar"
    data-action="breakpoint:sm@window->sidebar#animateOut breakpoint:md->sidebar#animateIn">
  </aside>
</body>

Callback names

There is also a way to configure the name of the callback functions to be called when a breakpoint changed. You could for example map the breakpoints to device types:

useBreakpoints(this, {
  callbackNames: (_prefix, breakpoint) => {
    return switch (breakpoint) {
      case 'xs': 'phone'
      case 'sm': 'tablet'
      case 'md': 'tablet'
      case 'lg': 'laptop'
      case 'xl': 'desktop'
      case 'xxl': 'desktop'
    }
  }
})

Mar 20 '21 15:03 marcoroth

I am absurdly excited to refactor about five different controllers to use this.

My main suggestion/ask also applies to useResize and useWindowResize:

I wish that in addition to the callbacks (breakpoints for useBreakpoints, plus resize and windowResize) it would be really helpful if the member variable(s) holding the current width or dimensions is accessible without the dimensions having to change.

What I mean is that when I'm using useResize I frequently end up creating a callback that only exists to memoize the value when the controller is first connected. eg:

  resize({ width, height }) {
    this.width = width
    this.height = height
  }

What I'm saying is that I wish this.width and this.height always pointed to the correct values without even having to define this callback. Of course, you can still define a callback! But in many cases, you just need to know the dimensions right now without waiting for a resize callback to occur.

Does that make sense?

Mar 21 '21 03:03 leastbad

There's often JavaScript features that are only active at given breakpoints (mobile navs, for ex), so definitely keen to have an easy way to trigger controller methods at different points. I've got a few concerns though:

The implementation fires a callback doing computation for every resize event. This might happen rarely on mobile/tablet devices (when tilting them), but those event fire at a very high frequency when resizing a browser window on desktop, which is just a waste. It could be worth at least throttling the listener or maybe let the browser do the job of detecting breakpoint changes using matchMedia. The later bit might be more the job of a useMatchMedia mixin, though. I'd personally prefer using that for triggering controller methods at different breakpoints: less code shiped to users to monitor resize, plus opening to handle non width based media queries (height, aspec-ratio, orientation, prefers-reduced-motion, prefers-color-scheme).
Given this is meant to be mixed into controllers, won't multiple breakpoint:changed events get fired if multiple controllers on the page call useBreakpoints, possibly triggering listeners of those events multiple times for the actions listening on @window? It feels like the mixin is trying to do two things at the same time:
1. Call methods of the controller as breakpoints change
2. Notify the whole page that breakpoints change
Sounds like that last bit is more the responsibility of the controller, to be registered only once in the application, I guess on the body, than the mixin.
The default implementation of the controller brings in a method for each of the breakpoints, which actuall app code might not need to use. I think at least making them no-ops will save bundling unnecessary code into apps. To reduce things further, maybe the mixin could check for method existence before trying to call? This would avoid having to actually implement those methods on the controller (L101, if I didn't misinterpret what's happening).
Will the unused Tailwind/Bootstrap breakpoint list be tree-shaken out of the build when using the other one to set the breakpoints? Usually there'll be only one of those used per project so no need to ship users the other one. If a project actually needs both (migration or such), they'll explicitely import them as necessary.
The documentation encourages to call #show or #hide based on breakpoints. Showing and hiding are usually more CSS concerns and I'm wary of setting precedents encouraging that. If we're imagining animated transitions (for ex. slide back the navigation nicely) maybe those method names could be renamed to animateIn/animateOut to highlight that it's actually something that requires some JS to run.

A couple of features that I think would be really handy:

global configuration so that you wouldn't need to remember to import the right set of breakpoints. Something like useBreakpoints.breakpoints = {/* hash of breakpoints */} in the main JS file which would make all subsequent calls of useBreakpoints() not need to pass the list of breakpoints
knowing which breakpoint was active before the change: Say I need to disable some JS for the mobile navigation on screens MD and wider. If I change from SM to MD I don't need to do anything, but if I change from LG to MD I do need to run some code.

Hope this is useful and, of course, happy to discuss further 😄

Mar 21 '21 11:03 romaricpascal

@romaricpascal you're correct about the barrage of events, but we shouldn't impose a debounce solution on the developer... especially when stimulus-use comes with useDebounce. I suspect we win via documentation instead of being heavy handed in the implementation.

I'm pretty sure that you only need to define callbacks for the breakpoints you want to respond to. Am I mistaken?

While I like the idea of knowing which breakpoint was active previously, I would caution against building features this way since it would be quite brittle; for example, if I click a button that changes the size of the window, it might skip intermediate steps.

Mar 21 '21 17:03 leastbad

Completely agreed on the breakpoint... callbacks, those should be left to the developer to chose whether to debounce, throttle or do whatever they please with. In the end, they won't trigger that often unless the user plays with their window at the edge of the breakpoint limit. I wasn't clear there and was actually refering to the internal callback used to monitor the screen width and compute whether to trigger the developer callbacks, line 109.

Good spot on breakpoint being skipped if you resize programmatically. And I guess developers can always do the tracking in their controller in a first place if needed using the breakpoint... callbacks, so might not be such a great idea to add it from the start.

Mar 22 '21 09:03 romaricpascal

Thank you @romaricpascal, I appreciate the detailed response!

I think the best way is if I answer to each list item individually:

That's right. I thought about throttling the internal callback too. I messed around with the throttle method from use-throttle but couldn't get it to work properly. Maybe I can take another look at that.

Re: the non width based media queries: I also thought about that, and I think it could also be worth to investigate how that could be implemented. There are two problems we need to solve
1. Which event should we listen for to track all the changes?
2. What happens if multiple breakpoints are "active". Should we just call the controller action for all active breakpoints? The reason I didn't investigate further and stayed with the basic width based breakpoints is because we can't really evaluate which one should be active. If we detect multiple min-width breakpoints with matchMedia, should it then call all matched ones? How do we know which one should be called first so the call order makes sense? In the order they are defined? Since both Bootstrap and Tailwind just use the "default" width based breakpoints by default (and not media queries per se) I wanted to go with that and keep it simple.
But it would be nice if you can specify any media query you can think of. Something like:
```
useBreakpoints(this, 
  breakpoints: {
    'dark': '(prefers-color-scheme: dark)',
    'light': '(prefers-color-scheme: light)',
    'landscape': '(orientation: landscape)',
    'portrait': '(orientation: landscape)',
    // ...
  }
)
```
But as you said, maybe this is something we could/should tackle in a useMatchMedia or useMediaQueries mixin. So we could also add the constraint, that multiple "queries" can be active/called.
That's also right. If you use the useBreakpoints multiple times it will fire the elements on their corresponding controller elements and the events will bubble up. But I think the responsibilities are right. It's how all the mixins behave in stimulus-use. We could introduce a bubbles options to prevent the event from bubbling up, so you don't end up with multiple event on @window. But you can also disable the events with dispatchEvent: false. So I guess that should be fine / up the developer.
The default implementation doesn't contain a method for each breakpoint. It doesn't even know about the breakpoints. this.call() (now on L104) handles that. It checks if the method exists on the controller and calls it if the method exists.
Re tree-shaking: I'm not sure about that one. But I would assume? But this probably also depends on the config in your application itself. Maybe @adrienpoly can expand on that topic.
That's a fair point. I just used the first thing that came to my mind, so it's a somewhat reasonable example. I can change that to animateIn/animateOut 👍🏼

For the features:

This is a good idea. I implemented that in https://github.com/stimulus-use/stimulus-use/pull/125/commits/bca1660fd15c581126bcc8e924cc97a1172a5b45
I also like this addition. This should be covered with: https://github.com/stimulus-use/stimulus-use/pull/125/commits/32b554be031085db32200cec62a30b2e7e293961

Thank you for your constructive comments!

Mar 22 '21 09:03 marcoroth

Thanks all for those contributions.

Lots have been said I will try to cover some outstanding questions/comments

Throttling

Yes lets keep this outside of the mixin. I agree that the current implementation with the resize event might fire extra callbacks but this is not cblocking for me and we can look in a second step to optimize this (see below).

useMatchMedia or useMediaQuery

I really like this idea to wrap this match media API into a mixin and as a second step refactor useBreakpoint to use this mixin. This would give more flexiblity for the various queries to listen too and still with useBreakpoint a comprehensive and easy to use mixin.

this.width & this.length getters

In response to @leastbad suggestion to have this.width and this.height getters for mixins such as useResizeand useWindowResize I am not really for it right now. While it is not yet possible as the callback are not configurable I think it would be nice to be able to include several of the same mixins within the same controller to track different nested elements. or one mixin that will not track just one element but a collection of elements (targets). In such cases this.width is not uniq anymore.

tree shaking

I am pretty confident that unused code is removed during tree shaking. Builds are tested with (https://github.com/Rich-Harris/agadoo) before release.

show/hide animateIn/animateOut

useTransition uses enter/leave maybe we could stick to that

Tests

I think the easier would be to use the Cypress setup to test that?

Thansk again for you contribution

Mar 23 '21 09:03 adrienpoly

I just put up #128 which should cover the use-cases @romaricpascal has pointed out ☺️

Mar 24 '21 00:03 marcoroth

@marcoroth is this still a draft ? It looks good to me?

Jun 18 '21 13:06 adrienpoly

I don't think it's worth having this mixin when we have the useMatchMedia mixin which was introduced in #128

Sep 04 '23 22:09 marcoroth

stimulus-use stimulus-use copied to clipboard

Introduce `useBreakpoints` mixin

Basic Usage

Default Breakpoints

The power of events

Callback names

Throttling

useMatchMedia or useMediaQuery

this.width & this.length getters

tree shaking

show/hide animateIn/animateOut

Tests

stimulus-use
stimulus-use copied to clipboard