Add tweet block for embedding tweets #754

notnownikki · 2017-05-10T13:40:03Z

This commit polishes the Placeholder component to better accommodate error messages. Also rephrases a little verbiage in both the embed and Tweet blocks, and fixes a typo in a CSS class.

…to add/twitter-block

jasmussen · 2017-05-10T16:36:20Z

Added some polish to this. Looks good!

As you can see I had a little merge adventure where for some reason I didn't have the latest files. Hopefully that all worked out.

notnownikki · 2017-05-10T16:40:36Z

Ah yes, I pushed up changes forgetting that you might have already cloned... muscle memory from old working patterns using Gerrit :)

This looks great, thank you! I'm going to go through the code again and prepare it for review.

Updated the header on the imported resizable iframe component

aduth · 2017-05-10T17:07:29Z

blocks/library/embed/index.js

@@ -31,7 +31,7 @@ registerBlock( 'core/embed', {
 		if ( ! url ) {
 			return (
 				<Placeholder icon="cloud" label={ wp.i18n.__( 'Embed URL' ) } className="blocks-embed">
-					<input type="url" className="placeholder__input" placeholder={ wp.i18n.__( 'Enter URL to embed here...' ) } />
+					<input type="url" className="components-placeholder__input" placeholder={ wp.i18n.__( 'Enter URL to embed...' ) } />


It was already there, but we should update the translated text to use the ellipsis character … instead of three dots.

aduth · 2017-05-10T17:59:52Z

blocks/library/tweet/index.js

+ * Internal dependencies
+ */
+import { registerBlock, query } from '../../api';
+import Sandbox from '../../../components/sandbox';


With the components directory, the idea becomes that we treat these more like external modules, but unlike npm modules, they're first-party in the sense that they'd be WordPress-maintained.

So two things:

We can import this from components/sandbox

We should move this under a separate WordPress dependencies docblock above this one

There's some more context documented here, which I'm sensing could probably be moved somewhere more obvious:

https://github.com/WordPress/gutenberg/blob/master/docs/coding-guidelines.md#javascript

Also: #716

aduth · 2017-05-10T18:12:50Z

blocks/library/tweet/index.js

+		}
+		doFetch( url, setAttributes, setState ) {
+			setState( { fetching: true, error: false } );
+			jQuery.ajax( {


Hmm, this could be related to #691. Generally I tend to discourage network requests in the component itself. The line is blurred here because the result of this request is not really valuable outside the context of the component.

The use of jQuery also sparked my interest. It's handy and readily accessible. We'll probably want to think on our options here some more; probably outside the scope of this pull request specifically.

At the very least, with mind toward #742 and eliminating globals*, we'll probably want to import this as a module and configure Webpack to treat this as an external (reference). I believe it should be enough to add 'jquery': 'jQuery' there, and then you can import jQuery from 'jquery'; at the top of this file.

* For consistency and to enable changing the source from which jQuery is resolved.

Yeah, this is a little yucky, but it's purely in the editor, and only used to show the tweet as it would appear in the post. The saved html is actually just the tweet's url, as wp will process that anyway, doing the same call this javascript does.

I'll address the globals issue.

I'm also a bit conflicted with relying on jQuery here. It's true we'll have it available in WordPress for the foreseeable future, but coupling an async request with a whole library doesn't feel great for the longer term and portability of editor code.

aduth · 2017-05-10T18:19:29Z

blocks/library/tweet/index.js

+			super( ...arguments );
+			this.fetchTweet = this.fetchTweet.bind( this );
+			this.state = {
+				url: this.props.attributes.url,


It's discouraged to create copies of props in state because it muddies the fact that the source of truth is the prop itself, leaving you responsible for maintaining when that prop changes. And evidenced by a lack of componentWillReceiveProps or componentDidUpdate, it appears we're not.

Instead, where you're currently using this.state.url, you should try to reference this.props.attributes.url directly instead.

And depending on if we want the component to trigger another request if the URL ever changes, it'd be good to bind to componentDidUpdate to check if the URL has changed and fire doFetch again.

So when the button is clicked (or form submitted) I'd use a reference to the text input to set the url, instead of having it set when the text input changes?

So when the button is clicked (or form submitted) I'd use a reference to the text input to set the url, instead of having it set when the text input changes?

Ah, I think I overlooked that. With that in mind, it can be fine to create a copy.

Here's an archived (some outdated syntax) article which explains more of the context and highlights this exception:

https://github.com/facebook/react/blob/8cac523/docs/tips/10-props-in-getInitialState-as-anti-pattern.md

However, it's not an anti-pattern if you make it clear that the prop is only seed data for the component's internally-controlled state:

Since we don't have as much control over the naming of the incoming prop, except if we were to create an intermediary component or rename the attribute (both of which don't seem ideal), I think it'd be fine to ignore the initialUrl recommendation here.

I'll add a comment in the constructor to make it clear what we're doing here.

aduth · 2017-05-10T18:20:45Z

blocks/library/tweet/index.js

+							(
+								<Button
+									isLarge
+									onClick={ this.fetchTweet }>


What if the user types a URL in the text input and hits enter? Instead of binding to onClick, I'd recommend considering wrapping the input and button in a <form>, changing the button to type="submit", then binding to the forms onSubmit handler.

aduth · 2017-05-10T18:39:10Z

components/resizable-iframe/index.js

+const debug = debugFactory( 'gutenburg:resizable-iframe' ),
+	noop = () => {};
+
+export default React.createClass( {


React.createClass is deprecated and this needs to be rewritten as a wp.element.Component class.

aduth · 2017-05-10T18:39:24Z

components/resizable-iframe/index.js

+		return {
+			onLoad: noop,
+			onResize: noop,
+			title: uuid()


What's the value of a randomly generated title?

eslint flags an error unless all iframes have a unique title. This seemed the simplest way to get that, unless there's some ID unique to each instance that I can grab? (It wouldn't surprise me, but I couldn't see one)

Ah, that's a confusing rule description:

<iframe> elements must have a unique title property to indicate its content to the user.

https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/iframe-has-title.md

The value of the rule is not in ensuring guaranteed uniqueness, but rather that users of screen readers are easily able to identify and distinguish the contents of the iframe.

I expect the ESLint rule won't enforce its uniqueness, only that it's present.

To the purpose of the rule, I'd say we should set the title from the Twitter block as something like "Twitter Tweet embed". With that in mind, ResizableIframe should probably accept title as a prop and apply it to the rendered iframe.

The referenced guideline has some more useful context: https://dequeuniversity.com/rules/axe/1.1/frame-title

Maybe also cc @afercia on this one, in case I'm misspeaking. The guidelines do seem to strongly encourage that "no titles are repeated". We could include the Tweet's URL in the title as added uniqueness, but I don't sense this is valuable for screen reader users. The oEmbed response does include some details, like Tweet author and the raw html (from which the tweet's content could be parsed, though not easily).

Even the tweet author doesn't help that much, if for example, you're embedded two tweets by the same author... Perhaps "Embedded tweet from " is a good compromise though? It would be easy enough to parse that out of the url.

aduth · 2017-05-10T18:40:45Z

components/sandbox/index.js

+/**
+ * External dependencies
+ */
+import React from 'react';


Similar issues in this file as the previous; wp.element in place of React and avoid React.createClass (it's not available in wp.element anyways).

aduth · 2017-05-10T18:41:21Z

components/sandbox/index.js

+	displayName: 'Sandbox',
+
+	propTypes: {
+		html: React.PropTypes.string,


PropTypes is moved out of React as of version 16. I'd expect this is logging a warning in the console. We should probably just drop propTypes here altogether.

aduth · 2017-05-10T18:41:57Z

post-content.js

+			'<!-- /wp:core/embed -->',
+
+			'<!-- wp:core/tweet -->',
+			'<div>https://twitter.com/automattic/status/777261837335326720</div>',


Per previous comment about returning rawContent as the block's url attribute, we should first remove the div wrapper.

notnownikki · 2017-05-11T12:02:14Z

New commit pushed up, should address all the issues.

…to add/twitter-block

notnownikki · 2017-05-17T09:42:25Z

This is getting superseded by #816 , which uses server side rendering for embedded content, and doesn't use jQuery at all.

…

On Wed, May 17, 2017 at 10:41 AM, Matias Ventura ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In blocks/library/tweet/index.js <#754 (comment)>: > + }, + + edit: class extends wp.element.Component { + constructor() { + super( ...arguments ); + this.fetchTweet = this.fetchTweet.bind( this ); + this.state = { + url: this.props.attributes.url, + html: '', + error: false, + fetching: false, + }; + } + doFetch( url, setAttributes, setState ) { + setState( { fetching: true, error: false } ); + jQuery.ajax( { I'm also a bit conflicted with relying on jQuery here. It's true we'll have it available in WordPress for the foreseeable future, but coupling an async request with a whole library doesn't feel great for the longer term and portability of editor code. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#754 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADKSsodx6LZdAyC2D52gEEb9OzIOj5vWks5r6sCvgaJpZM4NWrde> .

mtias · 2017-05-17T09:44:01Z

@notnownikki should this one be closed then?

notnownikki · 2017-05-17T09:46:03Z

Yes, closing it now, was just getting the other PR into a working state so work could carry on there.

Superseded by #816

Tweet block - WORK IN PROGESS! Don't merge or review or anything.

3cd38d1

notnownikki added the [Status] In Progress Tracking issues with work in progress label May 10, 2017

notnownikki requested a review from jasmussen May 10, 2017 13:40

Tweet block - WORK IN PROGESS! Don't merge or review or anything.

abc74cd

notnownikki force-pushed the add/twitter-block branch from 3cd38d1 to abc74cd Compare May 10, 2017 16:19

jasmussen added 3 commits May 10, 2017 18:29

Polish placeholder error message.

5fa3cb8

This commit polishes the Placeholder component to better accommodate error messages. Also rephrases a little verbiage in both the embed and Tweet blocks, and fixes a typo in a CSS class.

Merge branch 'add/twitter-block' of github.com:WordPress/gutenberg in…

8c38062

…to add/twitter-block

Merge latest changes.

09814e5

Tweet block

eedb7d7

Updated the header on the imported resizable iframe component

notnownikki changed the title ~~Tweet block - WORK IN PROGESS! Don't merge or review or anything.~~ Add tweet block for embedding tweets May 10, 2017

notnownikki added [Feature] Blocks Overall functionality of blocks and removed [Status] In Progress Tracking issues with work in progress labels May 10, 2017

aduth requested changes May 10, 2017

View reviewed changes

Fixes from code review

ce8fc70

notnownikki added 6 commits May 11, 2017 13:07

Lint fixes

5fd1bb9

Add jQuery

0aad93d

i18n fix for sandboxed iframe titles

5ce13f6

Merge branch 'master' into add/twitter-block

a43001f

Fixed iframe title after some a11y discussions

c223f18

Merge branch 'add/twitter-block' of github.com:WordPress/gutenberg in…

ff12c83

…to add/twitter-block

notnownikki mentioned this pull request May 12, 2017

Server side rendering API endpoint #780

Closed

mtias added the [Status] In Progress Tracking issues with work in progress label May 17, 2017

notnownikki closed this May 17, 2017

aduth mentioned this pull request May 22, 2017

Discussion: Block attributes initialization #865

Closed

aduth mentioned this pull request May 25, 2017

Our Data Approach #902

Closed

aduth mentioned this pull request Jun 15, 2017

Render shortcode previews within Gutenberg #1054

Closed

ntwb deleted the add/twitter-block branch August 5, 2017 01:22

snyk-bot mentioned this pull request Mar 12, 2022

[Snyk] Security upgrade simple-git from 2.48.0 to 3.3.0 MaxMood96/gutenberg#62

Open

yaseenaljamal mentioned this pull request Sep 13, 2023

[Snyk] Upgrade valtio from 1.7.0 to 1.11.2 yaseenaljamal/gutenberg#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tweet block for embedding tweets #754

Add tweet block for embedding tweets #754

notnownikki commented May 10, 2017

jasmussen commented May 10, 2017

notnownikki commented May 10, 2017

aduth May 10, 2017

aduth May 10, 2017

aduth May 10, 2017

notnownikki May 10, 2017

mtias May 17, 2017

aduth May 10, 2017

notnownikki May 10, 2017

aduth May 10, 2017

notnownikki May 11, 2017

aduth May 10, 2017

aduth May 10, 2017

aduth May 10, 2017

notnownikki May 10, 2017

aduth May 10, 2017

aduth May 10, 2017

notnownikki May 11, 2017

aduth May 10, 2017

aduth May 10, 2017

aduth May 10, 2017

notnownikki commented May 11, 2017

notnownikki commented May 17, 2017 via email

mtias commented May 17, 2017

notnownikki commented May 17, 2017

Add tweet block for embedding tweets #754

Add tweet block for embedding tweets #754

Conversation

notnownikki commented May 10, 2017

jasmussen commented May 10, 2017

notnownikki commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnownikki commented May 11, 2017

notnownikki commented May 17, 2017 via email

mtias commented May 17, 2017

notnownikki commented May 17, 2017