-
Notifications
You must be signed in to change notification settings - Fork 842
Jetpack Forms: cleanup dashboard migration code #43825
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
base: trunk
Are you sure you want to change the base?
Jetpack Forms: cleanup dashboard migration code #43825
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
01b44e4 to
46686be
Compare
46686be to
8bb753d
Compare
c0c4be0 to
9aeddde
Compare
3b48cbd to
e8e1245
Compare
9aeddde to
973ca3b
Compare
e8e1245 to
ce02411
Compare
973ca3b to
185f555
Compare
ce02411 to
4b0d868
Compare
185f555 to
5482344
Compare
a43d653 to
d074545
Compare
6ca04d0 to
a3a6a16
Compare
|
This PR has been marked as stale. This happened because:
If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation. If the PR is not updated (or at least commented on) in another month, it will be automatically closed. |
7f51231 to
68dd321
Compare
dd051e7 to
6de2cf5
Compare
68dd321 to
ab9ad30
Compare
6de2cf5 to
2e2f369
Compare
ab9ad30 to
ccc10f3
Compare
…ed conditional code
… don't offer classic view anymore
4d80d93 to
0bf2756
Compare
| * | ||
| * @return string | ||
| */ | ||
| private function append_tab_to_url( $url, $tab, $is_classic_view ) { |
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.
Any specific reason to convert this to a static function? No big deal. Just curious. We should be sure we catch any other places the function is used (if relevant).
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.
Yes, we don't want to be instancing the dashboard switch class when we don't need it, it's a simple mechanism. And previous changes remove the initial instantiation of the class, so there's no more a way to call it as a class method, only static. The same function exists on Dashboard class. The original idea was to delete the method (or the entire class), this is the safer approach.
edanzer
left a comment
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.
Changes look sane. I tested this in both self hosted and WordPress.com environments, since menu logic is a bit different in those places. Everything is working well for me in both environments. Also tests pass.
I added a few questions.
I think the main concern with this will be whether there are edge cases where users are still using the old / classic Feedback page. It's tricky to identify those cases (if they exist) and test them.
@CGastrell knows this menu space very well. So if he feels comfortable with my questions, and given everything is working, I think we'd be good to merge this. Still holding no approving though, since I'm not sure on edge cases myself yet.
WIP
Part of FORMS-133
Proposed changes:
This PR removes all code related to the dashboard and menu item migration
Other information:
Jetpack product discussion
No
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Sadly these removals don't have a very consistent way of being tested. Run tests with
jetpack test php packages/forms.Then navigate wp-admin. See that Jetpack > Forms works fine. See that installing
polldaddyplugin will trigger theFeedbackmenu on the sidebar.Sync to a JN site and a Simple site, verify nothing breaks on Calypso.