Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Dashboard: Rename class/feature to remove conflict with legacy dashboard widget #2138

Merged
merged 2 commits into from
May 8, 2019

Conversation

timmyc
Copy link
Contributor

@timmyc timmyc commented Apr 30, 2019

Fixes #2137

This branch renames the WC_Admin_Dashboard to WC_Admin_Analytics_Dashboard to prevent the name collision that currently results in the legacy Woo Core dashboard widget from being displayed.

Screenshots

image

Detailed test instructions:

  • On master of wc-admin installed/activated, note the bug by visiting the wp-admin dashboard and seeing that the WooCommerce Status widget is not shown ( see screen grab above )
  • Install/npm start this branch, revisit the wp-admin dashboard and see that it is now shown
  • Visit the WooCommerce Admin dashboard and verify it is still working

@timmyc timmyc requested a review from a team April 30, 2019 17:56
Copy link
Contributor

@jeffstieler jeffstieler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and simple! :shipit:

config/core.json Outdated
@@ -2,7 +2,7 @@
"features": {
"activity-panels": false,
"analytics": false,
"dashboard": false,
"analytics-dashboard": false,
"dashboard/customizable": false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename this flag to analytics-dashboard/customizable too?

@timmyc
Copy link
Contributor Author

timmyc commented May 6, 2019

@jeffstieler is this branch still working for you? I can no longer see the dashboard on this branch - I attempted rebasing but still no love. master works fine.

@jeffstieler
Copy link
Contributor

@timmyc I'm not seeing the dashboard anymore 😕

@psealock
Copy link
Collaborator

psealock commented May 7, 2019

I'm not seeing the dashboard anymore 😕

I'm not sure how this branch ever worked without updating this line too:

if ( window.wcAdminFeatures.dashboard ) {

@Tobias-Conrad
Copy link

@jeffstieler @justinshreve I was one of the admins which can not live without the WOO dashboard in the WP Dashboard.
I would like to contribute with testing, if a solution is ready please provide a download link.
At the moment it looks like, not all place of the dashboard issue have been seen ;-)
Goal could be to have this fix in the next update.

@timmyc
Copy link
Contributor Author

timmyc commented May 7, 2019

Good catch @psealock guessing while testing this I had a cached build locally. I was noticing there is a lag between a file save, and changes being reflected in my local build with npm start running.

I applied the fix, and also changed the name of the name to analytics-dashboard/customizable as well per @justinshreve 's feedback.

Copy link
Collaborator

@psealock psealock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@justinshreve
Copy link
Collaborator

justinshreve commented May 8, 2019

Good catch @psealock guessing while testing this I had a cached build locally. I was noticing there is a lag between a file save, and changes being reflected in my local build with npm start running.

It's possible that you maybe didn't exit and restart npm start after making the change, which means the old flag would have been true still? So the next time you went to test it, the correct flag kicked in.

@timmyc
Copy link
Contributor Author

timmyc commented May 8, 2019

It's possible that you maybe didn't exit and restart npm start after making the change, which means the old flag would have been true still?

Yep absolutely this was it. And that explains some of the choice words I uttered while trying to get the PR working again yesterday - 😆

@timmyc timmyc merged commit 3211fc9 into master May 8, 2019
@timmyc timmyc deleted the fix/2137 branch May 8, 2019 16:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WC_Admin_Dashboard class name conflict
5 participants