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

Add very basic embed block #424

Merged
merged 1 commit into from
Apr 20, 2017
Merged

Add very basic embed block #424

merged 1 commit into from
Apr 20, 2017

Conversation

ellatrix
Copy link
Member

This is a WIP. Needs UI to change the url, use the correct attribute and oEmbed.

@ellatrix
Copy link
Member Author

ellatrix commented Apr 14, 2017

Any ideas on how we should do oEmbed (fetching the src for the url attribute)?

@youknowriad
Copy link
Contributor

youknowriad commented Apr 17, 2017

From what I've understood on the discussions here #315 (comment) We can not store iframe in the post content. I think this block should be considered the first "dynamic" block.

By this I mean, its save method should return an empty string and all its attributes stored in the comment. And it should be processed on the server (close to how shortcodes work).

@ellatrix
Copy link
Member Author

Yeah, I know, but I was referring to the fetching of the iframe.

@youknowriad
Copy link
Contributor

@iseulde If we do the rendering server side, we could rely on the oEmbed spec, we won't be blocked by the CORS issue.

@ellatrix
Copy link
Member Author

Right, the rendering is already done server side with the admin-amax.php (same in current editor), but the content will come at a delay.

@jasmussen
Copy link
Contributor

Right, the rendering is already done server side with the admin-amax.php (same in current editor), but the content will come at a delay.

Can we show a block outline with a spinner, kind of like when you paste a link into the status box on Facebook?

@ellatrix
Copy link
Member Author

@joen Yeah, that sounds good, we do this in core as well. We could show the block in a loading state with the block icon or something. I think the API for registering blocks will need an update to allow this behaviour though (based on attributes, get more attributes async).

@ellatrix ellatrix force-pushed the add/embed-block branch 2 times, most recently from 1294b1e to 1dd1beb Compare April 17, 2017 09:36
@ellatrix
Copy link
Member Author

Should we add this basic version in the meantime?

@jasmussen
Copy link
Contributor

Should we add this basic version in the meantime?

Oh absolutely! 👍

@jasmussen
Copy link
Contributor

Looks good! We'll need to tweak and tune the default size, but I expect we'll revisit this at a later time. Seems god to me!

@ellatrix
Copy link
Member Author

Yeah we could look at that together with alignment. One tricky thing is to get the dimensions right.

@jasmussen
Copy link
Contributor

One tricky thing is to get the dimensions right.

I doubt we'll be able to. What does the editor do currently?

Probably we will want to decide some defaults here, and then let the user override them. A good bet would be to make them 16:9, as we want them responsive also. Then in an advanced inspector we could let a user manually input width/height.

Responsive could be done using this trick.

@ellatrix
Copy link
Member Author

What does the editor do currently?

We add a maxwidth query to the request for the provider, so in the case of YouTube, we'd get a width and heigh attribute back on the iframe. In the current editor maxwidth is the width the theme set for the content. As long as we have both, we know the ratio though, so we could set the size if it needs to be different for aligning. Not sure how to have a variable ratio in CSS though.

@ellatrix
Copy link
Member Author

It's even harder when the provider gives us a script... In that case we load the script in an iframe to isolate it. I think it will be hard to align that...

@jasmussen
Copy link
Contributor

It's even harder when the provider gives us a script... In that case we load the script in an iframe to isolate it. I think it will be hard to align that...

If we can apply a CSS style to the iframe directly, we just need to decide the aspect ratio and we can can responsively style it. But we'll cross this bridge when we get there.

@ellatrix ellatrix merged commit af45471 into master Apr 20, 2017
@ellatrix ellatrix deleted the add/embed-block branch May 1, 2017 21:39
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