-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fix design issues #290
Fix design issues #290
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.
Looks good, left a small feedback. We can perhaps also get design team to review the changes to confirm they don't find anything miss in parallel to QA.
Hey @Codeinwp/design-team, can you have a look here and see if there are any more required changes? 🙏 |
@cristian-ungureanu I tested it on a new taste instance and looks good. I only noticed that the hover color of the buttons is a bit off (we had discussed about a % of opacity if I recall correcty). But looking at the overall style of the wizard and the hover styles of other buttons, I think it would make sense for the blue buttons to use the I am referring to the hover states of these buttons:
Thank you for taking the time to follow-through these, and for putting so much attention. |
Thanks, @JohnPixle! The opacity was not really an option as it would have applied to the text of the button. |
hey @cristian-ungureanu https://demosites.io/ seems to be down now, so I need to wait for that before testing https://vertis.d.pr/i/iocG9V. |
@cristian-ungureanu I've tested on a normal and multisite, with various users. Found some issues on multisite, but they are not related to this PR, same thing happens for the current version as well: #292. Not sure if they should be addressed now or not. Regarding the current PR:
|
Hey @rodica-andronache, I managed to fix the first raised issue. The second one would require some changes in the demo data exporter repo. I think we should handle it separately as it's not that important and we can live with it for now. We can open a new issue for this and @HardeepAsrani will prioritize it as he sees fit. About the multisite issues you've mentioned, I think they are the same as you've mentioned in this PR, right? If yes, we can handle them there. Let me know your thoughts. |
onClick was passing the event which resulted in skip always being true.
@cristian-ungureanu I've tested the first issue and is fixed now. For the second one, I created a new issue here. The issues I've mentioned #292 do exist in this PR and in the current version of TPC |
Thank you for your awesome testing, Rodica. 🚀 If you agree, we can move this to "Ready to merge" and we'll handle everything else separately. |
Hey @cristian-ungureanu if you feel it's easier we can move this to ready to merge, and fix everything separately, but we'll have to test it again, before actually releasing this, because I think there are still issues. As I mentioned in the latest comment here Codeinwp/neve#4108 that issue is still not fixed with the latest commit from this PR. |
🎉 This PR is included in version 1.2.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
In this PR I address the following issues:
Note: After discussing this with Hardeep, the loading conditions for Onboarding were changed. The onboarding doesn't check for Neve's new users anymore. In order to load, here's what it checks for:
Test instructions
Closes #288.