Remove site specific block attributes from theme export #3899
Conversation
|
Thanks so much for working on this, it looks great! Do you think we should also remove the |
Good idea, done in 76e1636. I also added a filter for the media & text block. |
| if ( 'core/query' === $block['blockName'] && isset( $block['attrs']['queryId'] ) ) { | ||
| unset( $blocks[ $key ]['attrs']['queryId'] ); | ||
| $has_updated_content = true; | ||
| } |
There was a problem hiding this comment.
Are you sure queryId is a site-specific attribute? WordPress/gutenberg#42730 talks about removing ID's from the query block but I think it may have been referring to other attributes.
There was a problem hiding this comment.
@ntsekouras would you be able to weigh in here?
This PR is proposing to strip all query blocks of the queryId attribute if the theme is exported using the site editor. My understanding is that the attribute is site specific and shouldn't be set explicitly in theme templates.
There was a problem hiding this comment.
Sorry for the late reply here. queryId is used for proper Query Pagination when we have multiple Query Loop blocks, and there are a few nuances here..
If queryId doesn't exist and a Query Loop block is inserterd in an editor, it will auto-generate one. We usually want to strip/omit this in block patterns, because we know it will be opened in editor and therefore created during insertion of the block.
In themes now, if a template has more than one Query Loop blocks and we strip them to both, the pagination will be broken for these blocks. So a better strategy might be to preserve them.
There was a problem hiding this comment.
I see, thanks for the feedback, I agree in that case it's a better approach to leave the queryIds in. Updated in 642879b.
There was a problem hiding this comment.
@carlomanf @ntsekouras How do you feel about unsetting taxQuery attribute on query block?
A query block with a tax query "taxQuery":{"category":[7]} might give empty results in the new site.
Do you see any issues removing it or setting it to null?
There was a problem hiding this comment.
This makes sense. Similar props would be the parents, author and exclude(even if this one is not exposed in the UI.
|
Lets remove |
| unset( $blocks[ $key ]['attrs']['ref'] ); | ||
| $has_updated_content = true; | ||
| } | ||
| if ( 'core/image' === $block['blockName'] || 'core/cover' === $block['blockName'] && isset( $block['attrs']['id'] ) ) { |
There was a problem hiding this comment.
| if ( 'core/image' === $block['blockName'] || 'core/cover' === $block['blockName'] && isset( $block['attrs']['id'] ) ) { | |
| if ( ( 'core/image' === $block['blockName'] || 'core/cover' === $block['blockName'] ) && isset( $block['attrs']['id'] ) ) { |
Trac ticket: https://core.trac.wordpress.org/ticket/57537
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.