Live Preview: dismiss sticky notice after activating the theme (season 2) #84730
Conversation
matticbot
added
the
[Status] Needs Review
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
| // Subscribe to the core store, so that previewingThemeSlug can be updated when the theme is activated. | ||
| // We need this hack because the currentlyPreviewingTheme() function is not a hook. | ||
| select( 'core' ); | ||
| return currentlyPreviewingTheme(); |
There was a problem hiding this comment.
We use isResolving( 'activateTheme' ) to check whether the previewing theme is activating. Maybe we can use the same trick?
In addition, Gutenberg uses history.replace to update the query striing. Therefore, I guess we can use the location returned by the useLocation hook to detect the changes.
WDYT?
There was a problem hiding this comment.
We use isResolving( 'activateTheme' ) to check whether the previewing theme is activating. Maybe we can use the same trick?
Could you elaborate the trick? isResolving is a selector from the core store, which we already subscribe here 😄
In addition, Gutenberg uses history.replace to update the query striing. Therefore, I guess we can use the location returned by the useLocation hook to detect the changes.
I also considered this but it requires @wordpress/router, which is not installed by the wpcom-block-editor currently. Does it make sense to add this package for this purpose?
There was a problem hiding this comment.
I do not fully understand Arthur's intended meaning, but, for reference, I'm sharing the PR that started using isResolving( 'activateTheme' ). 🙂 WordPress/gutenberg#55658
There was a problem hiding this comment.
Could you elaborate the trick? isResolving is a selector from the core store, which we already subscribe here 😄
Instead of subscribing the core store, we can subscribe that value to trigger the update. Subscribing the core store may trigger the update all the time 🤔
Does it make sense to add this package for this purpose?
I think it's fine 🙂
There was a problem hiding this comment.
I checked with @arthur791004 and we verified that the current code is fine: it will trigger updates (e.g. rerender) when the value that's returned by useSelect() (i.e., previewingThemeSlug) changes, which is exacly what we want 😄
I looked into @wordpress/router again, but it looks like useLocation() is a private API, which needs unlocking. I think we don't need this complication for now!
There was a problem hiding this comment.
Sorry for reintroducing the bug and thank you for fixing it!
I'm approving this PR but I'm curious about Arthur's idea. #84730 (comment)
| // Subscribe to the core store, so that previewingThemeSlug can be updated when the theme is activated. | ||
| // We need this hack because the currentlyPreviewingTheme() function is not a hook. | ||
| select( 'core' ); | ||
| return currentlyPreviewingTheme(); |
There was a problem hiding this comment.
I do not fully understand Arthur's intended meaning, but, for reference, I'm sharing the PR that started using isResolving( 'activateTheme' ). 🙂 WordPress/gutenberg#55658
fushar
force-pushed
the
live-preview-broken-activate
branch
from
f3c4a4a
to
29ed5b4
Compare
fushar
force-pushed
the
live-preview-broken-activate
branch
from
29ed5b4
to
a382468
Compare
github-actions
bot
removed
the
[Status] Needs Review
Proposed Changes
This recent PR:
resurfaced a bug which have been previously fixed by:
The fix was subtle, so it can be easily missed. This PR reintroduces the fix, with code comments.
This PR also fixes a bug where we ALWAYS call the theme API (with null ID) event though we're not previewing anything.
Reviewers are encouraged to review commit-by-commit :)
Testing Instructions
widgets.wp.cominstall-plugin.sh wpcom-block-editor live-preview-broken-activate)widgets.wp.com.Pre-merge Checklist