Skip to content

Conversation

@ellatrix
Copy link
Member

@ellatrix ellatrix commented Jun 5, 2017

See #1002. Props @westonruter.

@ellatrix
Copy link
Member Author

ellatrix commented Jun 5, 2017

Note: the iframe-overlay class is not used anywhere atm.

@ellatrix ellatrix requested review from jasmussen and mtias June 6, 2017 11:10
@jasmussen
Copy link
Contributor

jasmussen commented Jun 6, 2017

Nice! Seems to work as intended. That is, when you click it the first time, it doesn't start, it selects the block. Nice. Clicking the second time, starts the video.

Are we sure we want the latter? Do we want to let you play the video in the editor at all? I have no strong opinion here, and I defer to those of you with good arguments on hand.

If we do want to allow it, though, I noticed that the invisible area to the left and right of the inline toolbar is unclickable no matter any preceeding clicks. This area:

screen shot 2017-06-06 at 13 15 25

Not a deal breaker, but if we do want to allow the video to play, we should consider making that area clickable also.

General 👍 though!

@ellatrix
Copy link
Member Author

ellatrix commented Jun 6, 2017

I'll look into the issue. Yeah, maybe let's see how this goes, and if we still want to block interacting with selected iframes at a later point, we can. :)

@ellatrix
Copy link
Member Author

ellatrix commented Jun 6, 2017

Okay, I have to admit that it would not be as straightforward as I thought. It looks like we're better off with some kind of wrapper component like <ClickToPlay message="..." icon="..." /> because embedding a tweet does not create an iframe. Or just a wrapper component to block interaction entirely for the time being.

@ellatrix
Copy link
Member Author

ellatrix commented Jun 6, 2017

I'll close this PR as the approach does not accomplish what was intended.

@ellatrix ellatrix closed this Jun 6, 2017
@ellatrix ellatrix deleted the fix/iframe-click branch June 6, 2017 12:35
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.

3 participants