-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
There was a problem hiding this 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.
I am going to test these today, thanks both! 🚀 |
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:
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. 🚀 |
@JohnPixle I've updated the naming, let me know if it's all good now. |
@HardeepAsrani it works fine, thanks! @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:
I think this was also one of your initial suggestions 👍🏻 It got me freaked out this one 😓 |
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:
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! |
@JohnPixle I've made the changes. |
🎉 This PR is included in version 1.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.