Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Featured image block contains invalid HTML #17754

Merged
merged 1 commit into from Oct 10, 2019

Conversation

jorgefilipecosta
Copy link
Member

Description

Fixes: #17739

This PR changes the responsive wrapper component to use a span instead of a div. The reason is that the component is used inside a button and having a div inside a button is not valid HTML.

How has this been tested?

I added a featured image and I checked it showed correctly on the panel.
I clecked the image and verified the media library opened.
I was able to select a new image with success.

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Oct 3, 2019
@@ -16,12 +16,12 @@ function ResponsiveWrapper( { naturalWidth, naturalHeight, children } ) {
paddingBottom: ( naturalHeight / naturalWidth * 100 ) + '%',
};
return (
<div className="components-responsive-wrapper">
<div style={ imageStyle } />
<span className="components-responsive-wrapper">
Copy link
Contributor

Choose a reason for hiding this comment

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

We're changing a generic component to address the needs of a specific one. Not suggesting it's a good approach but maybe we should first think if it does make sense for all usage? Any thoughts on that?

Also, I think with React hooks, these generic behavior can be written in a different way, hooks taking refs as argument maybe. So I'm wondering if there's an alternative solution here that would make this even more generic.

Copy link
Member Author

Choose a reason for hiding this comment

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

From the design perspective, this component is not changing at all I guess the only practical change is that it can be used in more places while still producing valid HTML.

maybe we should first think if it does make sense for all usage

We only have the featured image usage in core, so we can not easily judge other use cases. A span is something that we can use almost anywhere the restriction that what children we can nest inside when creating this PR I thought this was supposed to be used in images as even the code referred to imageStyles. But on a second thought I guess someone may pass a div or a paragraph as a child of this component and in that case, we would produce invalid HTML.

So the possibilities we have to make this more generic are a flag that allows choosing div or span or even a tagName that allows specifying the element.
Or something that looks into the children and if is something that can be nested in a span (e.g: image, another span or pure text) we use a span otherwise we use a div.

Also, I think with React hooks, these generic behavior can be written in a different way, hooks taking refs as argument maybe.

I'm not understanding how hooks would help. The idea is this component also exposing a hook the parent uses?
Because the component itself can not use a hook with a reference because to have a reference it needs to render something to the dom and to render something it already needs to know if it should render a div or a span.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right this is like a use-case where hooks might not make a lot of sense, thanks for explaining.

Can we maybe just add a boolean isInline prop to switch? Thoughts?

@jorgefilipecosta jorgefilipecosta force-pushed the fix/featured-image-contains-invalid-html branch from 666edb8 to 32cf6ae Compare October 10, 2019 16:55
const childrenProcessed = cloneElement( children, {
className: classnames( 'components-responsive-wrapper__content', children.props.className ),
} );
if ( isInline ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's the exact same code, can't we just use const TagName = isInline ? "span" : "div" to avoid duplicating it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it seems to reduce duplicate code, I update following your suggestion 👍

@jorgefilipecosta jorgefilipecosta force-pushed the fix/featured-image-contains-invalid-html branch from 32cf6ae to 524c46e Compare October 10, 2019 17:56
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Thanks

@jorgefilipecosta jorgefilipecosta merged commit 401ed23 into master Oct 10, 2019
@jorgefilipecosta jorgefilipecosta deleted the fix/featured-image-contains-invalid-html branch October 10, 2019 21:30
@youknowriad youknowriad added this to the Gutenberg 6.7 milestone Oct 14, 2019
@bobbingwide
Copy link
Contributor

This has broken the display of the Featured Image. Issue being raised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Featured image button contains a div which is invalid HTML
3 participants