-
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
chore: migrate colors to generic ones [closes #19] #21
Conversation
@Codeinwp/design-team Here, can you help us to make sure this works as intended or any changes are required? Thank you! |
The build file from above is not available for download, in order to do the testing. Probably it expired. |
@abaicus @HardeepAsrani After adding this PR, without making any change, the homepage colors changed: before the PR: https://github.com/Codeinwp/raft/assets/67897586/3989992f-4120-446e-9ca8-629d41b37d23 after the PR: https://github.com/Codeinwp/raft/assets/67897586/ac3fa0ae-9c31-4d24-9d58-5bd652176ec8 |
@irinelenache Yes, that does happen at first but you can refresh the page and it should start to work like previously. Let me know how it goes for you. |
@HardeepAsrani After a few refreshes the frontend looked fine 👍 In the editor though, the colors are still changed, i tried to clear the cache, use incognito and Browserstack. You can check on this instance:
Let me know if there's any problem on my end 🙏 |
Thanks @irinelenache, seems to be an issue. Before we start working on it @abaicus, just wanted to know if this was a known side effect of this or there was work needed to fix this behavior, if you're aware? |
@HardeepAsrani I don't remember exactly what was happening here, unfortunately. IIRC, this was working ok when I submitted the PR initially. It might be that I just overlooked the bug in the editor. |
@HardeepAsrani ok, I tested the build that you sent me in Slack, and the problem seems to be that some patterns and templates make use of the old color slugs. This is a problem for existing users, if we can't have a default fallback 🤔. Also safe to assume that users will have already used the theme colors from the palette, so this will mess up their appearance. Any advise here? Worst case scenario we do not change the slugs, just add new colors with the raft prefix. Sorry, I should have seen that coming earlier 😞 |
Development
@HardeepAsrani let me know what your thought are here when you can, regarding the backwards compatibility of the color slugs. We need to sort this out relatively soon in order to use the proper colors when building the patterns 🚀 Thanks! |
Hey, I was able to reproduce it now. I added the migration slugs to the editor as well and now the patterns do look and behave just as expected in the editor. The only issue will be that Color Picker won't show the correct color. Personally, I won't see it as the biggest issue. Let me know your thoughts. |
I've also replaced the color slugs in the existing patterns & templates: 10fc68d - to avoid new users using old slugs. |
@HardeepAsrani Hey, so after some further thought after our late night discussion yesterday, I have concluded that it's better not to change the slugs. I have some bitter personal experience on doing such global-scope changes to themes and I think it will hurt us and our users more that what it is supposed to offer. Here's an outline of my thinking:
Why we wanted consistent color slugs across themes. As we discussed, we wanted to have a consistent color slug system across all our themes. This could have unlocked some flexibility in the future, like users being able to use the patterns across all the themes, and maintain consistency. When we were building Raft we created the slugs to be theme-specific, not framework-specific. I think we must own this decision and move forward, doing the best we can in a safe and scalable way from now on. Let me know what you think. I am open to discuss this more if you want. For now, I think we can stay with the raft- slugs. You need to undo-the commit that replaces the slugs in the theme templates and patterns, that you did yesterday. We can merge everything then into a ZIP and test it out. I have done pretty good progress with the patterns and layout material so far, and having a theme file with all changes consolidated would vastly help. Thanks! |
@JohnPixle Thanks for your thoughts. I don't understand why to undo that commit if we aren't going to go ahead with. Let me know. |
@HardeepAsrani sorry, let me clarify. Since we are not going to use the ti-slug and we are going to stay with the old raft-slug then the commit u did that replaces the slug to the templates and patterns is no longer valid right? That's what I meant 🙂. |
@JohnPixle Then rest of the code in this PR is also not relevant as it replaces the raft- slug in options. 😄 |
@HardeepAsrani right, got it sorry 🙈! |
Edit from Hardeep:
Testing: Make sure all the color variables are migrated after the activating the zip from this build and the theme looks the same before/after the zip file.