Interactivity API: Mark all core block stores as private #58722
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DAreRodz
added
[Type] Enhancement
DAreRodz
requested review from
artemiomorales,
michalczaplinski,
tellthemachines and
ajitbohra
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core SVNCore Committers: Use this line as a base for the props when committing in SVN: GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
47 tasks
DAreRodz
added
[Type] Bug
|
Size Change: +69 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
cbravobernal
approved these changes
Feb 6, 2024
Comment on lines
+114
to
+115
| // We can't initialize the lightbox until the reference | ||
| // image is loaded, otherwise the UX is broken. |
There was a problem hiding this comment.
Suggested change
| // We can't initialize the lightbox until the reference | |
| // image is loaded, otherwise the UX is broken. | |
| /* | |
| * The lightbox cannot be initialized until the reference | |
| * image is loaded, otherwise the UX is broken. | |
| */ |
Comment on lines
+130
to
+131
| // In most cases, this value will be 0, but this is included | ||
| // in case a user has created a page with horizontal scrolling. |
There was a problem hiding this comment.
Suggested change
| // In most cases, this value will be 0, but this is included | |
| // in case a user has created a page with horizontal scrolling. | |
| /* | |
| * In most cases, this value will be 0, but this is included | |
| * in case a user has created a page with horizontal scrolling. | |
| */ |
Comment on lines
+135
to
+139
| // We define and bind the scroll callback here so | ||
| // that we can pass the context and as an argument. | ||
| // We may be able to change this in the future if we | ||
| // define the scroll callback in the store instead, but | ||
| // this approach seems to tbe clearest for now. |
There was a problem hiding this comment.
Suggested change
| // We define and bind the scroll callback here so | |
| // that we can pass the context and as an argument. | |
| // We may be able to change this in the future if we | |
| // define the scroll callback in the store instead, but | |
| // this approach seems to tbe clearest for now. | |
| /* | |
| * The scroll callback is defined and bound here so | |
| * that we can pass the context and as an argument. | |
| * This may be changed in the future if the scroll | |
| * callback is defined in the store instead, but | |
| * this approach seems to be clearest for now. | |
| */ |
Comment on lines
+142
to
+145
| // We need to add a scroll event listener to the window | ||
| // here because we are unable to otherwise access it via | ||
| // the Interactivity API directives. If we add a native way | ||
| // to access the window, we can remove this. |
There was a problem hiding this comment.
Suggested change
| // We need to add a scroll event listener to the window | |
| // here because we are unable to otherwise access it via | |
| // the Interactivity API directives. If we add a native way | |
| // to access the window, we can remove this. | |
| /* | |
| * A scroll event listener is needed to be added to the window | |
| * here because otherwise in unaccessible via | |
| * the Interactivity API directives. If there is a native way | |
| * to access the window, this can be removed. | |
| */ |
Comment on lines
+152
to
+157
| // We want to wait until the close animation is completed | ||
| // before allowing a user to scroll again. The duration of this | ||
| // animation is defined in the styles.scss and depends on if the | ||
| // animation is 'zoom' or 'fade', but in any case we should wait | ||
| // a few milliseconds longer than the duration, otherwise a user | ||
| // may scroll too soon and cause the animation to look sloppy. |
There was a problem hiding this comment.
Suggested change
| // We want to wait until the close animation is completed | |
| // before allowing a user to scroll again. The duration of this | |
| // animation is defined in the styles.scss and depends on if the | |
| // animation is 'zoom' or 'fade', but in any case we should wait | |
| // a few milliseconds longer than the duration, otherwise a user | |
| // may scroll too soon and cause the animation to look sloppy. | |
| /* | |
| * Wait until the close animation is completed | |
| * before allowing a user to scroll again. The duration of this | |
| * animation is defined in the styles.scss and depends on if the | |
| * animation is 'zoom' or 'fade', but in any case, it needs to be | |
| * a few milliseconds longer than the duration, otherwise a user | |
| * may scroll too soon and cause the animation to look sloppy. | |
| */ |
Comment on lines
+305
to
+307
| // In the case of an image with object-fit: contain, the | ||
| // size of the <img> element can be larger than the image itself, | ||
| // so we need to calculate where to place the button. |
There was a problem hiding this comment.
Suggested change
| // In the case of an image with object-fit: contain, the | |
| // size of the <img> element can be larger than the image itself, | |
| // so we need to calculate where to place the button. | |
| /* | |
| * In the case of an image with object-fit: contain, the | |
| * size of the <img> element can be larger than the image itself, | |
| * so calculate where to place the button. | |
| */ |
Comment on lines
+315
to
+316
| // If it reaches the width first, keep | ||
| // the width and compute the height. |
There was a problem hiding this comment.
Suggested change
| // If it reaches the width first, keep | |
| // the width and compute the height. | |
| /* | |
| * If it reaches the width first, keep | |
| * the width and compute the height. | |
| */ |
Comment on lines
+324
to
+325
| // If it reaches the height first, keep | ||
| // the height and compute the width. |
There was a problem hiding this comment.
Suggested change
| // If it reaches the height first, keep | |
| // the height and compute the width. | |
| /* | |
| * If it reaches the height first, keep | |
| * the height and compute the width. | |
| */ |
Comment on lines
+137
to
+141
| // If focus is outside modal, and in the document, close menu | ||
| // event.target === The element losing focus | ||
| // event.relatedTarget === The element receiving focus (if any) | ||
| // When focusout is outsite the document, | ||
| // `window.document.activeElement` doesn't change. |
There was a problem hiding this comment.
Suggested change
| // If focus is outside modal, and in the document, close menu | |
| // event.target === The element losing focus | |
| // event.relatedTarget === The element receiving focus (if any) | |
| // When focusout is outsite the document, | |
| // `window.document.activeElement` doesn't change. | |
| /* | |
| * If focus is outside modal, and in the document, close menu | |
| * event.target === The element losing focus | |
| * event.relatedTarget === The element receiving focus (if any) | |
| * When focusout is outsite the document, | |
| * `window.document.activeElement` doesn't change. | |
| */ |
Comment on lines
+57
to
+61
| // If focus is outside search form, and in the document, close menu | ||
| // event.target === The element losing focus | ||
| // event.relatedTarget === The element receiving focus (if any) | ||
| // When focusout is outside the document, | ||
| // `window.document.activeElement` doesn't change. |
There was a problem hiding this comment.
Suggested change
| // If focus is outside search form, and in the document, close menu | |
| // event.target === The element losing focus | |
| // event.relatedTarget === The element receiving focus (if any) | |
| // When focusout is outside the document, | |
| // `window.document.activeElement` doesn't change. | |
| /* | |
| * If focus is outside search form, and in the document, close menu | |
| * event.target === The element losing focus | |
| * event.relatedTarget === The element receiving focus (if any) | |
| * When focusout is outside the document, | |
| * `window.document.activeElement` doesn't change. | |
| */ |
cbravobernal
reviewed
Feb 6, 2024
packages/interactivity/CHANGELOG.md
Outdated
| @@ -6,6 +6,10 @@ | |||
|
|
|||
| - Break up init with yielding to main to prevent long task from hydration. ([#58227](https://github.com/WordPress/gutenberg/pull/58227)) | |||
|
|
|||
| ### Bug Fixes | |||
|
|
|||
| - Avoid initializing private stores as public when they have initial state. ([#58722](https://github.com/WordPress/gutenberg/pull/58722)) | |||
There was a problem hiding this comment.
The PR title is different than the title here.
There was a problem hiding this comment.
You are right; it's confusing. I'm going to cherry-pick those commits and create another PR just for the bug fix.
DAreRodz
force-pushed
the
update/interactivity-api-block-stores-private
branch
from
1b6c600
to
3ed5952
Compare
|
Thanks for the review, @c4rl0sbr4v0! 🙏 Do you mind if we leave the comments suggestions for a different PR? EDIT: maybe to this one: #58718 |
No problem! |
|
Question about this, does this mean I am not able to for example build a custom block that is a child of the navigation block and access its context / store? I can think of a few instances where especially for the navigation block it would be very useful to be able to access that. Sorry if I'm misunderstanding. |
@fabiankaegy, it is exactly that. We wanted to limit the APIs we expose during this first iteration, although that could change in the next versions, depending on the potential use cases.
I'd love to hear more about those instances you have in mind. 😄 |
|
@DAreRodz Thanks for confirming. I can understand that reason but I am a little concerned what it will mean for extenders. The first example coming to mind is mega menus. With some changes that were made in 6.5 it is finally possible to extend the list of allowed blocks that can be used in the navigation block. But a megemenu for example would need to know whether the navigation area it is in is currently expanded or not. So it would need to access the context. I'll add some more examples later 👍 |
|
If the core data stores are being “locked”, that means that any site or project which uses them in the frontend will break with the release of 6.5, doesn't it? Will they remain accessible if the project uses nonce authentication to access the store? (I'm referring to |
|
@markhowellsmead like we chatted about on twitter. The data API stores and the WordPress Data stores have nothing to do with one another. So no they are unaffected. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Epic: #56803
Mark the store of all core blocks as private, preventing access to their content from other plugins.
Also, fix private store initialization when populating the initial state.
Why?
To avoid exposing internal data that shouldn't become a public API.
How?
Simply adding a
{ lock: true }option to each store definition. It's easier to see if the changes to whitespace characters are hidden.Testing Instructions
store()from the Interactivity API module.core/filestore. It should throw an error.core/image,core/navigationandcore/query. The result must be the same.core/searchstore. It should fail as expected.