-
Notifications
You must be signed in to change notification settings - Fork 798
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
Remove old Fly Panel for showing in some scenarios #37764
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
88bb3ff
to
31a2697
Compare
I should note that we last attempted to remove those panels, we had to revert as it was breaking navigation in some scenarios: #12454 Our work on nav-unification may have solved that potential problem on WoA sites, but it may be important to test this change on other self-hosted WordPress sites as well, to ensure we're not removing important options for site owners. |
Thanks for the info @jeherve |
dee5797
to
a5e5629
Compare
I updated the PR, fixing some broken scenarios (like mobile or missing log-out). I also created this diff (D151708-code) with some extra details. Btw, one test is failing on Github but I can't make it fail locally 🤔 cc @jeherve |
You'll need to update your PR to latest |
a5e5629
to
145523f
Compare
First, I reproduced the issue described in https://github.com/Automattic/dotcom-forge/issues/6809 on the Atomic Default site but not on Atomic Classic. After applying the patch, I observed that the issue was resolved ✅. FWIW, I noticed different behavior on Atomic Classic, which seems appropriate given the differences between Calypso and wp-admin:
I think this PR need more eyes for;
|
As I understand, we maintain the behavior of non-classic sites. |
I'm not sure I have a strong opinion one way or the other on this one. I could go either way. |
c32c440
to
251b159
Compare
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.
I'm seeing the old Fly Planel from Default sites only. After applying the patch it is fixed.
Also tested with self-hosted sites to make sure nothing is outright broken.
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.
I tested with the diff and PR and SSO enabled and disabled, Default and Classic View, self-hosted. I also tested logging out with SSO disabled on Atomic. ✅
f9e8536
to
2b7e774
Compare
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.
This looks good to me on self-hosted. The current UX in Jetpack latest stable isn't great, and this doesn't really make it worse, so I'd say let's go with it (after a rebase)
So excited to finally have this gone! :-) |
1fdda78
2b7e774
to
1fdda78
Compare
Fixes https://github.com/Automattic/dotcom-forge/issues/6809
Proposed changes:
Old fly panel was showing when clicking on WP & Profile in the masterbar when Jetpack is enabled but SSO is disabled.
Important
This PR should be merged after this diff (D151708-code) is merged since it remove the function of the Fly panel and this PR removes content.
Before:
Screencast.from.2024-06-07.21-25-48.mp4
After:
Screencast.from.2024-06-10.21-50-31.mp4
Does this pull request change what data or activity we track or use?
No
Testing instructions:
s0.wp.com
My Home
at WPcom