-
Notifications
You must be signed in to change notification settings - Fork 805
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 VR Gutenberg block #9935
Fix VR Gutenberg block #9935
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests well! 👍 I haven't seen any errors, and couldn't break it in my various way of testing.
I've left several small notes, nothing critical - the main thing i see is that we need to make sure fields are aligned and sized properly in any width.
@@ -6,11 +6,15 @@ | |||
overflow: hidden; | |||
} | |||
|
|||
.wp-block-jetpack-vr .components-placeholder__fieldset { | |||
justify-content: space-around; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
icon="format-image" | ||
label={ __( 'VR Image', 'jetpack' ) } | ||
className={ props.className } | ||
> | ||
<UrlInput | ||
<TextControl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any good reason to use TextControl
here? Is it because of the style
prop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the alternative? As @simison noted in the PR desc, UrlInput
is deprecated...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use <input>
like core-embed block does:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that might make sense.
<UrlInput | ||
<TextControl | ||
type="url" | ||
style={ { flex: '1 1 auto' } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this flex: '1 1 auto'
is really necessary here - could you expand on why we need it? On a quick test, when I remove it I don't see a difference.
Found out that `.min.js` does get generated, I was just building things the wrong way. :-)
This is automated check which relies on Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that this fixes the block -- it works again, according to testing instructions.
The block is obviously still in a rather raw state, but my vote is to merge this PR since I'd like to proceed to move it out of JP (#10331) and into Calypso (Automattic/wp-calypso#27873).
@jeherve so our plan here would be to:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to work well on my end. No more errors, the photos render well in the editor and on the frontend now.
I have not tested adding a block with an old version of Gutenberg and then updating Gutenberg and Jetpack though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing it 👍 💯
Current VR block is incompatible with latest Gutenberg 3.2.0:
The block was originally implemented in #9560
Changes proposed in this Pull Request:
UrlInput
withTextControl
Testing instructions:
yarn build
(or use un-minified version by settingdefine ( 'SCRIPT_DEBUG', true );
)https://en-blog.files.wordpress.com/2016/12/regents_park.jpg