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

feat: add iframe block #859

Merged
merged 37 commits into from
Oct 19, 2021
Merged

feat: add iframe block #859

merged 37 commits into from
Oct 19, 2021

Conversation

kariae
Copy link
Contributor

@kariae kariae commented Sep 7, 2021

add an iframe block that will let publishers to embed an iframe either by its URL or by uploading
the iframe archive

All Submissions:

Add an iframe block to let publishers embed iframes by adding the iframes URL or by uploading the iframes content.

How to test the changes in this Pull Request:

  1. Pull the branch related to this PR.
  2. Add a new post.
  3. In the new post add the iframe block.
  4. Add the iframe source URL and click on the embed button.
  5. Observe the iframe embed in the post.
  6. If set to full-screen mode in the sidebar, preview the post and observe that the iframe is filling all the page.
  7. On another post add again an iframe block, and this time upload a zip archive that contains the iframe files (HTML, CSS, images, ...).
  8. Observe the embed iframe in the post.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@kariae kariae requested a review from a team September 13, 2021 15:22
Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

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

Apart from the individual comments, I'm wondering whether the block name is not too technical, but I don't have any counter-propositions.

src/blocks/iframe/edit.js Outdated Show resolved Hide resolved
src/blocks/iframe/edit.js Show resolved Hide resolved
src/blocks/iframe/view.php Show resolved Hide resolved
src/blocks/iframe/edit.js Show resolved Hide resolved
src/blocks/iframe/edit.js Outdated Show resolved Hide resolved
src/blocks/iframe/view.php Outdated Show resolved Hide resolved
src/blocks/iframe/view.php Outdated Show resolved Hide resolved
src/blocks/iframe/view.php Outdated Show resolved Hide resolved
@thomasguillot
Copy link
Contributor

@kariae I've pushed some simple visual changes. But I'd like to see a few more.

I'm not a fan of the Embed Block placeholder style and I think it looks messy having a URL input and a Button on 2 different row. I much prefer the Image Block placeholder style.

For the Height/Width settings, can we use something similar to the Cover Block regarding the unit? At the moment it's hard-coded in the input.

I'm not sure we need to have the "Preview" state when the block is unfocused. The editor "jumps" when I click on the block. Plus, there's already a button in the toolbar to show the preview.

@kariae
Copy link
Contributor Author

kariae commented Sep 20, 2021

@thomasguillot thank you for the feedback.

I'm not a fan of the Embed Block placeholder style and I think it looks messy having a URL input and a Button on 2 different row. I much prefer the Image Block placeholder style.

If we do go with the Image Block placeholder style, I'll add an insert from URL button, when we do click on it do I need to show an input + button on the bottom or I'll open a popup with the URL input and a confirmation button?

For the Height/Width settings, can we use something similar to the Cover Block regarding the unit? At the moment it's hard-coded in the input.

I like this one, I'll add it.

I'm not sure we need to have the "Preview" state when the block is unfocused. The editor "jumps" when I click on the block. Plus, there's already a button in the toolbar to show the preview.

After using the block on some posts I can confirm that. I'll remove it :)

@thomasguillot
Copy link
Contributor

If we do go with the Image Block placeholder style, I'll add an insert from URL button, when we do click on it do I need to show an input+button on the bottom or I'll open a popup with the URL input and a confirmation button?

Popup, just like the Image Block placeholder.

Screenshot 2021-09-20 at 13 45 46

@adekbadek
Copy link
Member

What's the status here? I was waiting for an approval from @thomasguillot as a sign that design-wise the work is done.

@kariae
Copy link
Contributor Author

kariae commented Oct 8, 2021

@adekbadek all good from my side, if @thomasguillot 👍 I fix the conflicts and this should be good to go.

@kariae
Copy link
Contributor Author

kariae commented Oct 8, 2021

Both JS and SCSS staged linter failed after the merge:

Error: Could not find "@wordpress/stylelint-config/scss". Do you need a configBasedir?

And:

wp-content/plugins/newspack-blocks/src/blocks/author-profile/edit.js
  19:2  error  Definition for rule '@wordpress/no-unsafe-wp-apis' was not found  @wordpress/no-unsafe-wp-apis

A first guess would be because the npm run lint:js:staged command is running on staged files only, though missing the configs, I'm not sure.

@thomasguillot
Copy link
Contributor

All good 👍
Made a few more changes in 247a9a3

includes/class-newspack-blocks.php Outdated Show resolved Hide resolved
src/blocks/iframe/iframe-placeholder.js Outdated Show resolved Hide resolved
src/blocks/iframe/edit.js Outdated Show resolved Hide resolved
src/blocks/iframe/edit.js Outdated Show resolved Hide resolved
src/blocks/iframe/edit.js Outdated Show resolved Hide resolved
src/blocks/iframe/edit.js Show resolved Hide resolved
src/blocks/iframe/view.php Outdated Show resolved Hide resolved
src/blocks/iframe/view.php Outdated Show resolved Hide resolved
kariae and others added 19 commits October 11, 2021 19:11
add an iframe block that will let publishers to embed an iframe either by its URL or by uploading
the iframe archive
change label text form from a question to normal text
when adding a fullscreen iframe to a post we'll show a notice saying that the iframe will take over
on the content on the post page (we don't want to preview this behavior as it will not let the user
user the editor)
set the UI as the Image Upload Block UI and enable getting archive assets from media library
change Upload to Embed since we do embed a URL not upload it
@kariae kariae requested a review from adekbadek October 14, 2021 09:37
Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

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

I've ran into this issue:

  1. Insert the block
  2. Choose "Embed from URL"
  3. Input the first letter
  4. -- the block changes mode to preview and the URL insertions is impossible

src/blocks/iframe/edit.js Outdated Show resolved Hide resolved
src/blocks/iframe/block.json Show resolved Hide resolved
A bug was introduced in 76f9021 where when we type the first letter in the URL field it's submitted,
moved the field state to the right place.
@kariae
Copy link
Contributor Author

kariae commented Oct 14, 2021

I've ran into this issue:

  1. Insert the block
  2. Choose "Embed from URL"
  3. Input the first letter
  4. -- the block changes mode to preview and the URL insertions is impossible

Nice Catch 👌

@kariae kariae requested a review from adekbadek October 14, 2021 17:36
Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

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

🙌

@kariae kariae merged commit 08a2712 into master Oct 19, 2021
@kariae kariae deleted the add/iframe_block branch October 19, 2021 14:18
matticbot pushed a commit that referenced this pull request Oct 19, 2021
# [1.41.0-alpha.1](v1.40.0...v1.41.0-alpha.1) (2021-10-19)

### Features

* add iframe block ([#859](#859)) ([08a2712](08a2712))
* **homepage-posts:** add a filter to enable duplicating posts ([#886](#886)) ([4daf27d](4daf27d))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.41.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Oct 19, 2021
# [1.41.0](v1.40.0...v1.41.0) (2021-10-19)

### Features

* add iframe block ([#859](#859)) ([08a2712](08a2712))
* **homepage-posts:** add a filter to enable duplicating posts ([#886](#886)) ([4daf27d](4daf27d))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.41.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

5 participants