Skip to content

Conversation

@youknowriad
Copy link
Contributor

closes #856

@youknowriad youknowriad added the General Interface Parts of the UI which don't fall neatly under other labels. label May 23, 2017
@youknowriad youknowriad self-assigned this May 23, 2017
@youknowriad youknowriad requested review from ellatrix and jasmussen May 23, 2017 10:32
@jasmussen
Copy link
Contributor

Hmm, I'm getting some JS errors as soon as I open the sidebar now:

screen shot 2017-05-23 at 12 48 17

Do I need to npm update?

@youknowriad
Copy link
Contributor Author

@jasmussen Oh sorry about this, I was testing with a saved post. It should be ok now.

@jasmussen
Copy link
Contributor

Love it. 👍 from me.

Also 🔥 this is going fast!

@ellatrix
Copy link
Member

ellatrix commented May 23, 2017

Looks good to me too. I think there might be some accessibility concerns, but maybe best to fix in a separate PR. Maybe the area needs a label?

@ellatrix
Copy link
Member

Hm, also seeing this:

screenshot 2017-05-23 13 18 16

Also adds an aria-label to the excerpt
@youknowriad
Copy link
Contributor Author

Good catch @iseulde Addressed those in 061937f

</a>
</PanelBody>
);
/* eslint-enable jsx-a11y/label-has-for */
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch (Ah copy/paste)

excerpt: getEditedPostExcerpt( state ),
} ),
( dispatch ) => {
return {
Copy link
Member

@aduth aduth May 23, 2017

Choose a reason for hiding this comment

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

Minor: Should we be consistent with how we're creating object return values between mapStateToProps and mapDispatchToProps, i.e. either with an explicit return in both, or implied here:

( dispatch ) => ( {
	onUpdateExcerpt( excerpt ) {
		dispatch( editPost( { excerpt } ) );
	},
} )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! do you have a preference. I'm leaning towards the explicit return (less magic) but I'm ok with both

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 feel strongly. I personally prefer the compact version, but understand that it can be a bit more difficult to interpret (largely because of the need to wrap parentheses for an implicit object return value).

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

Labels

General Interface Parts of the UI which don't fall neatly under other labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar: Add Excerpt panel

5 participants