Update breakpoints usage syntax

[colmtuite]

Myself, @hadihallak, and @StephenHaney were chatting about Modulz having to parse mediaQueries and ignore non-breakpoint media queries.

We eventually decided it's no big deal.

But it made me realise that non-breakpoint media queries don't belong in the tokens object. Tokens are for abstracting logic like 300px or hsl(208,100%,50%).

So I suggested moving them to utils. @hadihallak said they don't belong there either, because utils should output CSS properties.

So we eventually agreed it makes sense to split them into 3 separate objects.

tokens: {
  breakpoints: {
    1: '520px',
    2: '900px',
    3: '1200px',
    4: '1800px',
  }
},
utils: {
  marginX: (config) => (value: number | string) => ({
    marginLeft: value,
    marginRight: value,
  }),
},
mediaQueries: {
  motion: (rule) => `@media (prefers-reduced-motion) { ${rule} }`,
  hover: (rule) => `@media (hover: hover) { ${rule} }`,
  dark: (rule) => `@media (prefers-color-scheme: dark) { ${rule} }`,
  light: (rule) => `@media (prefers-color-scheme: light) { ${rule} }`,
}

This also allows us to simplify the breakpoint syntax by only supporting min-width breakpoints.

[peduarte]

Cool with me too 👌

[jjenzz]

I like how this simplifies things but are we sure that we only want to support min-width? That seems a bit limiting to me. I know that I've had to use min-height/max-height for some of my more creative projects and it just feels like this should be up to consumers... some prefer desktop first.

[peduarte]

@jjenzz Agreed, but I think it should be enough for launch at least. Then we have more time to think through in more depth, and see what users want/need

[jjenzz]

UPDATE

Utils will support media queries when the inline media query work is merged: https://codesandbox.io/s/romantic-colden-c61i8

---

To summarise our Slack discussion... I really think our focus should be to allow utils to support @ rules (which I believe would automatically work when inline media query support is added) and then leave breakpoints as they were. It doesn't make sense to me to take away beneficial flexibility if we've already built it.

const { styled } = createStyled({
	tokens: { 
		'$red': 'tomato' 
	},
	breakpoints: {
		'@short': rule => `@media(max-height: 300px) { ${rule} }`,
	},
	utils: {
		_motion: config => value => ({
			'@media (prefers-reduced-motion)': value,
		}),
		_hover: config => value => ({
			'@media(hover: hover)': {
				':hover': value,
			}
		}),
	}
});

const Button = styled('button', {
	color: 'blue',
	_hover: { color: '$red' },
	'@short': { /* whatever */ }
});

Then we just document recommendations:

• prefix or uppercase your custom properties and tokens for clarity
• mixin stuff belongs in utils like marginX and @media(hover: hover)
• breakpoints go in breakpoints

[hadihallak]

Here is an update on where this one is headed:

• we're not going to add mediaQueries as utils can replace them as @jjenzz pointed out.
• breakpoints are going to be reworked to avoid them being confused as media queries:

This outputs min-width:

breakpoints: {
  1: '200px' 
}

Or you can do a full one

breakpoints: {
  1: { minWidth: '200px', maxHeight: '400px' }
}

[peduarte]

Thanks for the update @hadihallak

[braposo]

Sorry to jump into the discussion but I guess that now everything is open! 😅

I was going to open a new issue/request asking if we couldn't simplify the breakpoints API as it seems a bit redundant to always have to include the rules when defining a breakpoint.

At the same time I realised that doing something like this would also work:

breakpoints: {
  bp1: (rule) => `@media (min-width: 520px) { ${rule} }`,
  bp2: (rule) => `@media (min-width: 900px) { ${rule} }`,
  noGridSupport: (rule) => `@supports not (display: grid) { ${rule}}`
}

So the breakpoints API seems to handle any CSS at-rule that you put in there, which is a bit confusing (but very useful!).

From @hadihallak comment I assume that going forward you're planning on changing the breakpoints API to something like:

breakpoints: {
  1: { minWidth: '200px', maxHeight: '400px' }
}

I think this has a couple of issues:

1. it's another syntax to "load" into your brain. Not a big deal here but can still be confusing to lots of people, especially if they are trying to migrate from a CSS-based system

2. You can't do more "rich" queries like @media only screen and (orientation: landscape) and (min-width: 520px). Also probably not a big deal for most people but it feels like using this syntax doesn't have the same feature parity as @media queries.

So, have you considered deprecating breakpoints and use a cssRules or simply queries (probably not the best names) entry in the config that handles any media query? You could also remove the need of having a function if that's not necessary by the core system.

Something like this:

queries: {
  bp1: `@media (min-width: 520px)`,
  bp2: `@media (min-width: 900px) and (max-width: 1280px)`,
  noGridSupport: `@supports not (display: grid)`
}

This would consolidate all cases instead of splitting them between breakpoints and utils configs as shown in @jjenzz example, which "forces" the users to use two different syntaxes to achieve a similar thing.

For most this would just be used for breakpoints definition but it could have any other at-rules for more advanced uses.

Just something for you to consider, hope you don't mind! 😬

[jjenzz]

@braposo Thanks for your input on this. I believe what you suggest should already work, however, we treat the entries in breakpoints differently because breakpoints come with specificity concerns that other at-rules don't have. This "bucket" allows us to differentiate those rules which we currently move into a separate style tag.

The discussion around reworking breakpoints API was to make it clearer that it should only contain breakpoints (because we treat them differently).

@hadihallak @christianalfoni I'm curious if it's an issue to have other at-rules moved into this new style tag too though? I had also hoped for the API proposed by @braposo during our internal discussions so I'm curious if it's worth revisiting. We've made lots of changes since our original discussions.

[braposo]

hey @jjenzz thanks for the reply! I must admit that it felt a bit weird to comment on this as I'm sure lots has been discussed outside of this issue so thanks for shedding some light into the decisions behind this.

It makes sense to separate breakpoints to give them a "special treatment" comparing with the other at-rules. I don't think this was very clear from the docs, which is completely expected at this point btw, so my initial question was more to understand the reasoning behind isolating the breakpoints.

One thing that is still confusing to me (and you mentioned that as well) is why does it have to be a completely different syntax from the "traditional" media queries declaration, especially when it seems that it doesn't provide the same features and it will need to be converted to a CSS media query anyway at some point.

Hopefully @hadihallak @christianalfoni can give a bit more info on that!