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 focus issue on embed block #517

Merged
merged 3 commits into from
Apr 26, 2017
Merged

Fix focus issue on embed block #517

merged 3 commits into from
Apr 26, 2017

Conversation

ellatrix
Copy link
Member

To test, try to select an embed block.

@ellatrix ellatrix added this to the Prototype Parity milestone Apr 26, 2017
@ellatrix
Copy link
Member Author

I'll address #483 here too. See also #316.

@ellatrix ellatrix self-assigned this Apr 26, 2017
<iframe src={ url } title={ title } />
{ caption || isSelected ? (
<div className="iframe-overlay">
<iframe src={ url } title={ title } />
Copy link
Member

@aduth aduth Apr 26, 2017

Choose a reason for hiding this comment

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

Per our Slack conversation, perhaps in order to show an icon in the overlay, we could render the overlay within the relative wrapper, only if the block has not received focus:

<figure>
	<div className="embed-wrapper">
		{ ! focus && (
			<div className="embed-overlay">
				<Dashicon icon={ /* ... */ } />
			</div>
		) }
		<iframe { /* ... */ } />
	</div>
	{ /* ... */ }
</figure>

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this could work too. I'm just wondering how you know how high the iframe is.

Copy link
Member

Choose a reason for hiding this comment

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

The height? I think it should work just the same as you have now, if you apply position: relative; to .embed-wrapper and position: absolute; top: 0; right: 0; bottom: 0; left: 0; to .embed-overlay.

Copy link
Member Author

Choose a reason for hiding this comment

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

How is .embed-overlay the same as iframe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, sorry @aduth, I did not see .embed-wrapper.

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of a setting, they could just add a wrapper to place it where they want. Maybe this could be a separate React component like <IframeOverlay>.

Copy link
Member

@aduth aduth Apr 26, 2017

Choose a reason for hiding this comment

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

I think we still could. In the above example, there's little difference between the .embed-overlay and the :before pseudo-element.

The problem with @rileybrook / @jasmussen's mockups at #483 (comment) is that it assumes we know that the embed is a video type. How we get this information is still a little unclear at this point. The embed block's implementation might be able to determine its own content type through proposed APIs like the oembed/1.0/proxy endpoint, which could return the provider and let us compare against a "known" mapping of provider -> type. This is not great either, though even server-side we don't really have a good sense of the type of a supported embed provider.

Otherwise we could default to something generic.

Copy link
Member

Choose a reason for hiding this comment

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

Are we talking about having many different block types for various sorts of embeds? I'm not sure why this would be a common behavior across blocks, at least to the point of wanting to create an abstraction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I thought this invisible overlay was a good start for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

To your last comment, this overlay is now just to prevent clicks going through iframes, so I named it like that. Not sure if it's intended for wider use.

@@ -7,6 +7,7 @@ msgstr ""
msgid "Embed"
msgstr ""

#: blocks/library/embed/index.js:33
Copy link
Member

Choose a reason for hiding this comment

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

Appears the i18n tools are misbehaving again; I think the reference on the following line was meant to be removed. npm run build should clear it up.

@aduth
Copy link
Member

aduth commented Apr 26, 2017

This is looking good. Before we'd got on the same page regarding overlay behavior, I put together a CodePen demo, which I mention only because I'd found I needed to adjust the wrapper to inline-block and iframe to display: block; to ensure correct width and avoid the gap between the frame and bottom of overlay. Should we apply those same changes here?

This is the visual bounds of the overlay currently:

Overlay bounds

@ellatrix
Copy link
Member Author

Yeah, that's correct. We could either address this here, or when adjusting the dimensions. Also, when images are treated as blocks, I found it easier to style if it's display: block; as well. I wonder if we should set these styles for images, videos, iframes, etc. in the same place.

@aduth
Copy link
Member

aduth commented Apr 26, 2017

I'm fine to address separately if you'd like to merge as-is. It's not totally clear yet whether we'd need general block styling applied to images, videos, iframe, etc if that's what you're suggesting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants