Skip to content
This repository has been archived by the owner on Mar 22, 2023. It is now read-only.

feat: add breakpoints mixin and usage example. #36

Merged
merged 6 commits into from
Mar 8, 2023

Conversation

chrisbellboy
Copy link
Contributor

Changes proposed in this Pull Request

This is built on top of #34 (so please check that one out first).

Changes summary

  • Add abstracts folder.
  • Add default breakpoints variable in abstracts\_variables.scss.
  • Add 3x breakpoints mixins (greater than / less than / in between) in abstracts\_mixins.scss.
  • .wp-theme-style in layouts\_pages.scss tweaked to show usage examples of the mixins.

@tommusrhodus tommusrhodus added the in progress A work-in-progress - not ready for merge! label Feb 8, 2023
@tommusrhodus
Copy link
Contributor

@chrisbellboy We've approved #34 and will be merging (thanks for that!) and we're most of the way there to approving this one also. Though looking for some feedback if you would?

  1. Whilst your listed breakpoints make total sense in terms of devices, they don't align with the gutenberg / wp-admin breakpoints that get used for things like the wp-block-columns block forcing stacking, which happens at $break-medium: 782px;
  2. Your 3 breakpoint mixins look great, where did these originate from? Are these your general go-to mixins for breakpoints?
  3. I like the error message functionality of the mixins, is it possible to automate the available breakpoints message? e.g using a map over $breakpoints so that if a project requires more/less to be added/removed, the error message automatically updates with that info?

@tommusrhodus tommusrhodus added the question Further information is requested label Feb 21, 2023
@tommusrhodus tommusrhodus self-assigned this Feb 22, 2023
@chrisbellboy
Copy link
Contributor Author

@tommusrhodus No worries and thanks for the feedback 👍

  1. Ah great point! That definitely would make more sense. I'll tweak it to use those breakpoints.
  2. Yeah this is pretty much my go-to for new projects. I started them a long time ago and they've evolved over time as I stumbled upon nuggets here and there! This latest version took inspiration from Bootstrap's responsive mixin and a random article that I read some time ago, must've been on medium or something (can't find it any more!).
  3. Nice idea, I'll have a play around and see if I can get it to work that way 👌

@ahegyes
Copy link
Contributor

ahegyes commented Feb 22, 2023

Ah great point! That definitely would make more sense. I'll tweak it to use those breakpoints.

I was actually wondering if those breakpoints can be overwritten in a theme.json file or in some other relatively easy way. I definitely think that we need to sync up with Gutenberg, but I am wondering if Gutenberg itself might also be able to sync up with another source ...

@chrisbellboy
Copy link
Contributor Author

I'm back 🤓 and pushed a few more commits:

  1. Changed breakpoint names + values to match Gutenberg's.
  2. Warnings now map over $breakpoints to list the available ones.

I was actually wondering if those breakpoints can be overwritten in a theme.json file or in some other relatively easy way. I definitely think that we need to sync up with Gutenberg, but I am wondering if Gutenberg itself might also be able to sync up with another source ...

To make them sync up with the theme.json I initially thought I cracked it by just using custom vars in theme.json, i.e. something like:

// theme.json with `settings.custom.breakpoints`

// variables.scss:
$breakpoints: (
    xs: var(--wp--custom--breakpoints--mobile, 480px),
    sm: var(--wp--custom--breakpoints--small, 600px)...

But then learnt that unfortunately CSS vars can't be used in media queries 🙈

The only other idea I could think of was to import the theme.json values into SASS. This seems possible but might over-complicate the overall setup a bit. We'd need to use something like this package and then extend the webpack.config.js to use the importer.

What do you guys reckon? 🤔 If you have any other ideas, happy to give them a go too.

@tommusrhodus tommusrhodus removed the in progress A work-in-progress - not ready for merge! label Mar 1, 2023
@ahegyes
Copy link
Contributor

ahegyes commented Mar 1, 2023

It looks like overriding the media queries or making them dynamic is not yet supported by Gutenberg, but it is something that is being discussed.

Similarly, there is a W3C proposal which, among other things, will remove the limitation of var not working inside media queries, but it's still a draft and will need a while.

There is also an interesting discussion about using em units for breakpoints so there is definitely lots of debate going on about improving this. Weird that responsive customizability wasn't one of the core design principles of GB though imho...

Anyway, we'll likely have to revisit this issue once those issues/PRs are settled, but for the time being, I think hard-coding the breakpoints and keeping them in sync with the Gutenberg core is the way to go.


I like the changes to the breakpoint mixins. I liked Bootstrap's breakpoint-only mixin, but it really requires all their functions to pull-off, so I think it's better for us if we just stick with breakpoint-between and a bit of manual labor.

If anything, I'd just remove the comment like Warn breakpoint is undefined right before the one-line @warn command, or the Return media query one. The whole thing is kinda self-documenting and I think we only really need the mixin-level comment.

Also, if we want to encourage more modularity, we could create an abstracts/mixing/_breakpoints.scss file to host these mixins and simply import that inside _mixins.scss. But that is definitely overkill for the demo here.

@chrisbellboy
Copy link
Contributor Author

Hey @ahegyes That all sounds good to me 👌 I've removed the comments now and feel free to let me know if you think there should be any more tweaks.

@ahegyes
Copy link
Contributor

ahegyes commented Mar 8, 2023

Approved on March 8th meeting.

@ahegyes ahegyes merged commit 5e385c1 into a8cteam51:trunk Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants