Skip to content
This repository has been archived by the owner on Jun 7, 2022. It is now read-only.

Fixing an unwanted page refresh when using Woo Navigation #7615

Merged
merged 3 commits into from Sep 15, 2021

Conversation

joelclimbsthings
Copy link
Contributor

@joelclimbsthings joelclimbsthings commented Sep 2, 2021

I noticed that navigating with the Woo Navigation was resulting in page refreshes when it shouldn't. This was just a quick fix due to a breaking change a while ago with registerPlugin needing a scope parameter.

Note that my prettier wants to completely reformat the markdown files for some reason, which is obfuscating the diff. Just adding the scope parameter in the docs as well.

Detailed test instructions:

  • Enable Woo Navigation
  • Go to the Homepage
  • Then use the Woo Navigation to go to "Customers," or an analytics report
  • There should be no page refresh
  • If you want, comment out the added scope option and notice that the page refresh returns

@@ -93,4 +93,5 @@ const NavigationPlugin = () => {

registerPlugin( 'wc-admin-navigation', {
render: NavigationPlugin,
scope: 'woocommerce-admin',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once WC Admin merges to core, will this remain woocommerce-admin?

Another question deals with the purpose of scope. It was introduced so that a <PluginArea /> would only render Fill that are relevant in the context. In other words, don't render a Fill that is intended for Gutenberg's text editor inside Navigation's plugin area. If slotFill becomes more prevalent as the default method of extending Woo functionality, maybe we can refine this a step further by making the scope woocommerce-navigation so that other Woo Fill won't also be rendered here.

What do you think? This brings up questions of backward compatibility, but if this change to Gutenberg hasn't been included in the latest WP release, there may still be time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this insight @psealock . As I understand it, using a different scope would require us to also render another <PluginArea scope="woocommerce-navigation" /> component somewhere as well to support it? Which will also be true with any future scopes that we would add in wc-admin?

I definitely like the idea of narrowing the scope, since it seems cleaner. That said, the above step could get slightly messy if we continue to add more scopes over time.

Also, just to make sure I understand, in a way we're narrowing the scope by only having targeted Slots with fairly unique IDs (prepended with 'woocommerce_navigation_). We're only adding slots with these IDs, which would be 1:1 with a potential Fill. That would make it unlikely for conflicts as well I believe?

Copy link
Collaborator

@psealock psealock Sep 9, 2021

Choose a reason for hiding this comment

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

in a way we're narrowing the scope by only having targeted Slots with fairly unique IDs

yup thats right. However, the scope in question, that of <Plugin />, refers to scripts calling registerPlugin. Unscoped, any plugin that uses registerPlugin will have a fill rendered on the page. Fills whose id don't match a slot won't actually be visible, but they still get rendered to the page in a visibility: hidden div, which means all lifecycle events get expressed.

The introduction of scope here was intended to limit rendered fills which we know won't have a corresponding slot and avoid unnecessarily executing lifecycle events.

Copy link
Contributor

Choose a reason for hiding this comment

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

The introduction of scope here was intended to limit rendered fills which we know won't have a corresponding slot and avoid unnecessarily executing lifecycle events.

Interesting! Thanks for the context, @psealock!

I'm in favor of using woocommerce-navigation for the scope in that case. This also makes me wonder if we should audit our existing SlotFills and narrow down scope a bit where possible. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The introduction of scope here was intended to limit rendered fills which we know won't have a corresponding slot and avoid unnecessarily executing lifecycle events.

Ah, thanks for this insight. Makes perfect sense. I'd also agree on narrowing scope on the plugin level with that in mind, and I can shove the revisions for navigation into this PR 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just made this change in a369c7a @psealock @joshuatf

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just made this change in a369c7a @psealock @joshuatf

Wonderful, I think its the way to go.

I'm a not up to date on the state of slotFill in wc-admin, but a quick audit would be good to know if any other instances make use of this < PluginArea />.

This also makes me wonder if we should audit our existing SlotFills and narrow down scope a bit where possible. 🤔

Most definitely a worthwhile follow up 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're actually implementing a bunch of slot-fill in the new tasks refactor, so I'll do the same there as well 👍🏻

@joshuatf joshuatf added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed status: needs review labels Sep 9, 2021
@joelclimbsthings joelclimbsthings added status: needs review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. needs changelog entry labels Sep 13, 2021
@joelclimbsthings
Copy link
Contributor Author

Thanks for looking at this @psealock @joshuatf . I believe we're 🟢 on another review.

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.

Changes look good, 🚢

@joelclimbsthings joelclimbsthings merged commit c30b6c5 into main Sep 15, 2021
@joelclimbsthings joelclimbsthings deleted the fix/woonav-page-refresh branch September 15, 2021 16:42
ObliviousHarmony pushed a commit to woocommerce/woocommerce that referenced this pull request Mar 18, 2022
…e/woocommerce-admin#7615)

* Adding changelog

* Fixing page refresh on woo navigation

* Narrowing scope of navigation slot fills
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.

None yet

3 participants