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 embed block placeholder. #603

Merged
merged 3 commits into from
May 4, 2017
Merged

Add embed block placeholder. #603

merged 3 commits into from
May 4, 2017

Conversation

BE-Webdesign
Copy link
Contributor

Closes #551.

This PR adds a placeholder for the embed block. Currently it does not
function and merely serves as a step forward on the UI.

Testing Instruction:

Open the inserter menu and add an embed block to the editor. The embed
block should feature an input to submit the URL. This functionality
does not yet work but serves as a placeholder.

Note:

Someone needs to do the translation build, as Windows breaks it.

@jasmussen
Copy link
Contributor

jasmussen commented May 2, 2017

Nice work! ⭐️

Would you like some UI for that, and if so would you prefer I do it in a separate branch post-merge?

Edit: by UI work I just mean some fonts, and some very little tweaks here and there.

@BE-Webdesign
Copy link
Contributor Author

Would you like some UI for that

Yes please 😄

and if so would you prefer I do it in a separate branch post-merge?

Whatever works best for you.

@jasmussen
Copy link
Contributor

I pushed a little polish 👆

screen shot 2017-05-02 at 18 37 23

Thank you again!

@@ -22,6 +26,23 @@ registerBlock( 'core/embed', {
edit( { attributes, setAttributes, focus, setFocus } ) {
const { url, title, caption } = attributes;

if ( ! url ) {
Copy link
Member

Choose a reason for hiding this comment

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

@aduth thoughts on a separate default method for this? I think it would clean up edit implementation a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

! url is fine by me, it is a micro performance optimization as well!

Copy link
Member

Choose a reason for hiding this comment

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

I mean more moving the return into a separate function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I misunderstood. We should do that at some point for sure. I don't think we need to right now, but once the placeholder actually has to do something we will probably turn it into its own component.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know that this necessarily needs to be built-in, but I think we could surface better the fact that edit is truly a React component, and can itself branch into separate subcomponents up to the implementer's discretion.

@@ -22,6 +26,23 @@ registerBlock( 'core/embed', {
edit( { attributes, setAttributes, focus, setFocus } ) {
const { url, title, caption } = attributes;

if ( ! url ) {
return (
<div className="blocks-embed is-placeholder">
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a <Placeholder> reusable component. @jasmussen do we expect the design to share these commonalities between image/embed/video/etc placeholders?

Copy link
Member

Choose a reason for hiding this comment

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

Regarding this question and the above about splitting out into a separate method... I'm not sure we can know the answer until we implement the placeholders for a couple of other blocks. I vote we merge something like this structure and iterate later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The interface for embedding a URL and inserting from the media library will probably be different.

Copy link
Contributor

@jasmussen jasmussen May 2, 2017

Choose a reason for hiding this comment

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

This sounds like a reusable component. @jasmussen do we expect the design to share these commonalities between image/embed/video/etc placeholders?

Yes, the design for a placeholder block is always 90% the same. Always a block, always an icon and a title centered, and always some more options after that. Those more options are likely going to be different, as @BE-Webdesign suggests:

  • Image will have a "open media library" button
  • Embeds will have a URL option
  • We'll probably run into other things as well.

So maybe something like, and please correct my pseudo code:

<Placeholder icon="cloud" title="Embed">
... input field and button ...
</Placeholder>

Copy link
Contributor Author

@BE-Webdesign BE-Webdesign May 2, 2017

Choose a reason for hiding this comment

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

This could probably make use of the Slot Fill as well, probably overkill. But I think unifying a component would be a future issue.

@mtias mtias added [Feature] Blocks Overall functionality of blocks Design [Type] Task Issues or PRs that have been broken down into an individual action to take labels May 2, 2017
@nylen
Copy link
Member

nylen commented May 2, 2017

I updated the translation build. What's the breakage on Windows there - what error were you getting?

What's the path forward for making this actually work (e.g. if I paste a YouTube URL and click Embed, what else is needed to convert this into an actual YouTube embed?) Is this something that can be done here?

This is probably for a separate PR, but shouldn't I also be able to switch between the "embed preview" and "embed placeholder/edit" modes?

@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented May 2, 2017

@nylen

What's the breakage on Windows there - what error were you getting?

#597. Not broken per se, but it will break the .pot. It has to do with Windows path resolution \ vs /.

@nylen nylen force-pushed the add/embed-block-placeholder branch from 05b0e11 to 92ca047 Compare May 2, 2017 22:04
@nylen
Copy link
Member

nylen commented May 2, 2017

@BE-Webdesign beware that I rebased and force-pushed this branch to fix the translation build (again) and the resulting conflicts with master.

+1 for merging this soon, but what do you think about the remaining questions in #603 (comment) (what's left to actually make this work?)

@BE-Webdesign
Copy link
Contributor Author

I think making it actually work is best left to another PR. I can however do it in this one if need be.

We would need to handle the input, validate that a URL was entered as the input, handle a successful switch to the embedded content, and handle any errors in case the user enters an invalid embed URL that leads no where. I am unfamiliar with using iFrames and React and their performance characteristics, so if it seems choppy, potentially some performance tweaks will be necessary as well. We will also need a way to change the url on the "final" version, and apply the same input/error handling there.

BE-Webdesign and others added 3 commits May 3, 2017 20:38
Closes #551.

This PR adds a placeholder for the embed block.  Currently it does not
function and merely serves as a step forward on the UI.

**Testing Instruction:**

Open the inserter menu and add an embed block to the editor.  The embed
block should feature an input to submit the URL.  This functionality
does not yet work but serves as a placeholder.

**Note:**

Someone needs to do the translation build, as Windows breaks it.
- Add a bg color. This will receive further love later, as this block has surfaced a problem with the initial design.
- Use the default font stack
- Normalize the input fields for gutenberg
- Rephrase ever so slightly
- Reposition input field and embed button with flex
@BE-Webdesign BE-Webdesign force-pushed the add/embed-block-placeholder branch from 92ca047 to e61cf86 Compare May 4, 2017 00:39
@BE-Webdesign
Copy link
Contributor Author

Merge? Making this fully functional is kind of a big step forward and depends on other moving parts in the project.

@nylen nylen merged commit ee98411 into master May 4, 2017
@nylen nylen deleted the add/embed-block-placeholder branch May 4, 2017 01:00
@nylen
Copy link
Member

nylen commented May 4, 2017

Agreed. Thanks for working on this!

@nylen nylen mentioned this pull request May 4, 2017
@@ -48,4 +48,25 @@ body.toplevel_page_gutenberg {
@include break-small() {
padding-top: $header-height;
}

input, textarea {
Copy link
Member

Choose a reason for hiding this comment

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

This style is overriding the Text mode textarea styles, eliminating padding and changing font appearance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack, my bad, will fix.

jasmussen added a commit that referenced this pull request May 4, 2017
This is a PR to fix a regression I introduced in #603 (review).

This moves the styles to input fields to be scoped to blocks, and the sidebar, both of which will benefit from this style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants