Fix: Transforms: Audio and Video to file transform bug; Add: getTextElements function: #7830
Conversation
jorgefilipecosta
added
[Type] Bug
|
Could we also add this fix to the image block? It seems to be suffering the same problem:
|
jorgefilipecosta
force-pushed
the
fix/caption-conversion-audio-file
branch
2 times, most recently
from
0e10341 to
102daac
Compare
|
Hi @sarahmonster thank you for catching the problem in images the fix was added in this PR. |
|
Tested and this looks much improved! One small thing though... when you convert it to a file, the formatting is lost. If it's trivial to implement, it would be ideal if we could preserve the caption formatting on conversion. Otherwise, let's just ship this sooner—this is a big improvement. 👍 |
|
Hi @sarahmonster thank you for your tests 👍 The current implementation of the file block does not support formatting in that area. To solve that we would need to change the attributes specification to allow formatting and add UI controls that allow the users to format. I think adding formats there may make sense but I'm not certain if there was any reason/decision to not have formats there, maybe it is something we can discuss in another issue. |
There was a problem hiding this comment.
Right, I should have known that. 🙈 Let's go with it as-is then. This is pretty edge case anyway, but it may be worth considering adding the text formatting (at some point, as a separate PR) as per #3785.
This looks good from a design/fixes-the-bug sort of perspective; may warrant a quick 👀 from someone who likes JavaScript more than I do. 👍
| if ( isArray( element ) ) { | ||
| return flatMap( element, ( child ) => getTextElements( child ) ); | ||
| } | ||
| if ( element && element.props && element.props.children ) { |
There was a problem hiding this comment.
Isn't there some lodash helper for this kind of deep-check? get() or something?
I personally don't like it and prefer just using "raw" JS, but we use it elsewhere so we might as well here 🤷♂️
There was a problem hiding this comment.
| @@ -133,3 +133,24 @@ export function switchChildrenNodeName( children, nodeName ) { | |||
| return createElement( nodeName, { key: index, ...props }, childrenProp ); | |||
| } ); | |||
| } | |||
|
|
|||
| /** | |||
| * Function that returns an array of all the text elements | |||
There was a problem hiding this comment.
I don't understand this comment. I think "descendants" should be "descendant". Could you rephrase this maybe?
core-blocks/file/index.js
Outdated
| @@ -96,9 +97,10 @@ export const settings = { | |||
| type: 'block', | |||
| blocks: [ 'core/audio' ], | |||
| transform: ( attributes ) => { | |||
| const captionProcessed = getTextElements( attributes.caption ); | |||
There was a problem hiding this comment.
caption is a children value. It's not expected to be strictly compatible with element. The implementation of getTextElements in @wordpress/element and its usage with a children value contradicts this.
See #7934
…h custom formats; Add: getTextElements function The audio and video to file transform contain a bug where if formats were used (e.g: bold, italic, ...) the caption was not correctly parsed. Here we add a new function getTextElements that returns an array of all the text nodes present in an elements hieraraquy, and we make use of that function to solve the bug in the transform.
jorgefilipecosta
force-pushed
the
fix/caption-conversion-audio-file
branch
from
102daac to
27d845a
Compare
|
Closing as with the last changes to the formatting API this PR is now not relevant. |
The audio and video to file transform contain a bug where if formats were used (e.g: bold, italic, ...) the caption was not correctly parsed.
Here we add a new function getTextElements that returns an array of all the text nodes present in an elements hierarchy, and we make use of that function to solve the bug in the transform.
Fixes: #7626
How has this been tested?
Add a an audio block, add a caption with formats e.g: bold, italic etc.. Transform to file block and verify things work as expected.
Repeat the steps for the video block.