Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use spacing variables #67

Merged
merged 3 commits into from
Nov 20, 2023
Merged

feat: use spacing variables #67

merged 3 commits into from
Nov 20, 2023

Conversation

HardeepAsrani
Copy link
Member

@HardeepAsrani HardeepAsrani commented Nov 11, 2023

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

  • Make sure the spacing is consistent when switching from an old version of Raft to new version.
  • Existing sites shouldn't behave any differently from what they were already doing.
  • When making new pages and using Spacing settings, the spacing should be using the new spacing classes & values.

Closes #66.

@pirate-bot
Copy link
Contributor

Plugin build for e985037 is ready 🛎️!

Copy link
Contributor

@preda-bogdan preda-bogdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spacing changes look good code wise.

@JohnPixle
Copy link
Contributor

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

@JohnPixle
Copy link
Contributor

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
Copy link
Member Author

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

@pirate-bot
Copy link
Contributor

Plugin build for 67c95cd is ready 🛎️!

@JohnPixle
Copy link
Contributor

JohnPixle commented Nov 15, 2023

@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
Copy link
Contributor

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
Copy link
Member Author

@JohnPixle I've made the changes.

@pirate-bot
Copy link
Contributor

Plugin build for dedd907 is ready 🛎️!

@JohnPixle JohnPixle linked an issue Nov 16, 2023 that may be closed by this pull request
@HardeepAsrani HardeepAsrani merged commit 6d648df into development Nov 20, 2023
7 checks passed
@pirate-bot
Copy link
Contributor

🎉 This PR is included in version 1.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@pirate-bot pirate-bot added the released Indicate that an issue has been resolved and released in a particular version of the product. label Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Indicate that an issue has been resolved and released in a particular version of the product.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Making use of spacing variables
4 participants