Codeinwp / raft

Issues should be created in https://github.com/Codeinwp/otter-blocks
7 stars 2 forks source link

feat: use spacing variables #67

Closed HardeepAsrani closed 11 months ago

HardeepAsrani commented 11 months ago

Summary

It makes use of new spacing variables instead of using pixels. This PR also formats the theme.json file, hence the long diff. In hindsight, I should've used multiple commits but the diff viewer in VSCode didn't prepare me for Github. 😄

Only changes to the code were made here:

Before: https://github.com/Codeinwp/raft/pull/67/files#diff-160c735a986cf8c05e1f6b2e9ac89dd0410562d2fe5ccbdc93157bd92f3dc2bcL183-L186

After: https://github.com/Codeinwp/raft/pull/67/files#diff-160c735a986cf8c05e1f6b2e9ac89dd0410562d2fe5ccbdc93157bd92f3dc2bcR184-R224

Will affect visual aspect of the product

No

Test instructions

Closes #66.

pirate-bot commented 11 months ago

Plugin build for e985037f07fb58c90aa6b0f7358de61be0c65b65 is ready :bellhop_bell:!

JohnPixle commented 11 months ago

I am going to test these today, thanks both! 🚀

JohnPixle commented 11 months ago

Hey, in general it works ok, but I think there is an issue with the slugs 😬.

There is an instance WP where I tested, here is a magic login for you. And here is the page I worked on.

I was facing some issues with the 2xs and 2xl sizes, both in the editor and in the frontend.

I saw that at the frontend the code generated by wp was:

var(--wp--preset--spacing--2-xs) (notice the dash between 2-xs) but in the code, the variable's slug was "slug": "2xs"

After changing the slugs to from 2xs and 2xl to xxs and xxl accordingly, all the inconsistencies got away and everything is working fine. Am I hallucinating, or it might have been the case indeed?

I can edit the code if you want.

Thanks. Apart from this, the spacing vars are great. 🚀

HardeepAsrani commented 11 months ago

@JohnPixle I've updated the naming, let me know if it's all good now.

pirate-bot commented 11 months ago

Plugin build for 67c95cde7d469dfb543d7f3ecf3b7e46bbc22e4d is ready :bellhop_bell:!

JohnPixle commented 11 months ago

@HardeepAsrani it works fine, thanks! Here's a layout built with spacing variables: https://raft.vertisite.cloud/patterns/features/ 😀

⚠️ UPDATE:

@HardeepAsrani Actually, I discovered something, that we can hopefully prevent. If we change the slugs of the core spacing variables, this means that any layouts built with those old vars would be broken. It is safe to assume that in those 10K users, many of them would have tweaked that default spacing scale.

Do you think that if we maintain the core slugs it would make sense in this case?

This is the current active setup:

    --wp--preset--spacing--20: 0.44rem;
    --wp--preset--spacing--30: 0.67rem;
    --wp--preset--spacing--40: 1rem;
    --wp--preset--spacing--50: 1.5rem;
    --wp--preset--spacing--60: 2.25rem;
    --wp--preset--spacing--70: 3.38rem;
    --wp--preset--spacing--80: 5.06rem;

I think this was also one of your initial suggestions 👍🏻

It got me freaked out this one 😓

Screenshot 2023-11-15 at 1 14 27 PM

JohnPixle commented 11 months ago

I think I got into a solution if you agree, by reverting to the existing slugs with 20,30,40 etc.

I have used this in theme.json:

"spacingSizes": [
                {
                    "name": "XXS",
                    "size": "clamp(.4rem, 2vw, 0.5rem)",
                    "slug": "20"
                },
                {
                    "name": "XS",
                    "size": "clamp(.6rem, 2.5vw, 1rem)",
                    "slug": "30"
                },
                {
                    "name": "S",
                    "size": "clamp(1rem, 3vw, 1.5rem)",
                    "slug": "40"
                },
                {
                    "name": "M",
                    "size": "clamp(1.5rem, 4vw, 2rem)",
                    "slug": "50"
                },
                {
                    "name": "L",
                    "size": "clamp(2rem, 5vw, 3rem)",
                    "slug": "60"
                },
                {
                    "name": "XL",
                    "size": "clamp(3rem, 6vw, 4rem)",
                    "slug": "70"
                },
                {
                    "name": "XXL",
                    "size": "clamp(3rem, 7vw, 5rem)",
                    "slug": "80"
                }
            ],

I essentially kept the t-shirt size names (S,M,L etc), but used the existing slugs (20,30,40).

I just tested some layouts with with the older version of raft and it worked good, users will not see a major change.

If you agree with the code, you can replace it in the build.

Let me know if I can help with something!

HardeepAsrani commented 11 months ago

@JohnPixle I've made the changes.

pirate-bot commented 11 months ago

Plugin build for dedd90702da893cb458d3eaff102e88273ef8683 is ready :bellhop_bell:!

pirate-bot commented 10 months ago

:tada: This PR is included in version 1.1.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: