-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 optional link to image blocks #1772
Conversation
mtias
commented
Jul 6, 2017
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.
Works well in my testing.
blocks/url-input/index.js
Outdated
{ expanded && | ||
<form | ||
className="editable-format-toolbar__link-modal" | ||
onSubmit={ ( event ) => this.submitLink( event ) }> |
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 could just be onSubmit={ this.submitLink }
, no?
} | ||
|
||
render() { | ||
const { url, onChange } = this.props; |
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.
Wondering if url
should be managed in state, and the parent component instead binds to onSubmit
.
Maybe also, should url
be reset when toggling the expanded state?
My thinking is "url" is state relevant only so long as the input is expanded.
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 had that initially, but we also need it when collapsed to show the button state (dark gray), so it seems a prop is more accurate.
blocks/library/image/index.js
Outdated
return <img src={ url } alt={ alt } className={ `align${ align }` } />; | ||
return href | ||
? <a href={ href }><img src={ url } alt={ alt } className={ `align${ align }` } /></a> | ||
: <img src={ url } alt={ alt } className={ `align${ align }` } />; |
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.
Ideally we could avoid some duplication here by assigning the base img
tag to a variable, and wrap it only if the href
condition applies.
blocks/url-input/index.js
Outdated
<input | ||
className="editable-format-toolbar__link-input" | ||
type="url" | ||
required |
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.
How will this work when trying to un-set the URL from the image? Currently, it shows an error that the field is required, but simply toggling "back" will show that it's truly removed (only because we're updating the block attribute onChange
and not onSubmit
).