-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Media: Populate advanced editing with parsed media #2541
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,42 +2,62 @@ | |
* External depencencies | ||
*/ | ||
import React, { PropTypes } from 'react'; | ||
import { bindActionCreators } from 'redux'; | ||
import { connect } from 'react-redux'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { toggleEditorMediaAdvanced } from 'state/ui/editor/media/actions'; | ||
import { setEditorMediaEditItem } from 'state/ui/editor/media/actions'; | ||
import Dialog from 'components/dialog'; | ||
|
||
function EditorMediaAdvanced( { visible, toggleVisible } ) { | ||
return ( | ||
<Dialog isVisible={ visible } onClose={ toggleVisible }> | ||
WIP | ||
</Dialog> | ||
); | ||
} | ||
const EditorMediaAdvanced = React.createClass( { | ||
propTypes: { | ||
item: PropTypes.object, | ||
resetEditItem: PropTypes.func, | ||
insertMedia: PropTypes.func | ||
}, | ||
|
||
getDefaultProps() { | ||
return { | ||
resetEditItem: () => {}, | ||
insertMedia: () => {} | ||
}; | ||
}, | ||
|
||
EditorMediaAdvanced.propTypes = { | ||
visible: PropTypes.bool, | ||
toggleVisible: PropTypes.func | ||
}; | ||
getButtonSettings() { | ||
return [ | ||
{ | ||
action: 'confirm', | ||
label: this.translate( 'Save' ), | ||
isPrimary: true, | ||
onClick: () => this.props.insertMedia( 'Updated' ) | ||
} | ||
]; | ||
}, | ||
|
||
render() { | ||
const { item, resetEditItem } = this.props; | ||
|
||
EditorMediaAdvanced.defaultProps = { | ||
visible: false, | ||
toggleVisible: () => {} | ||
}; | ||
return ( | ||
<Dialog | ||
isVisible={ !! item } | ||
buttons={ this.getButtonSettings() } | ||
onClose={ resetEditItem }> | ||
ID { item && item.media.ID } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ID seems pretty standard, but maybe it's worth a translation? I could imagine places where we would want some more natural expression for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is a WIP mostly to demonstrate that the parsing is working as expected. This will eventually be replaced with a more full-featured form (see #307). |
||
</Dialog> | ||
); | ||
} | ||
} ); | ||
|
||
export default connect( | ||
( state ) => { | ||
return { | ||
visible: state.ui.editor.media.advanced | ||
item: state.ui.editor.media.editItem | ||
}; | ||
}, | ||
( dispatch ) => { | ||
return bindActionCreators( { | ||
toggleVisible: toggleEditorMediaAdvanced | ||
}, dispatch ); | ||
return { | ||
resetEditItem: () => dispatch( setEditorMediaEditItem( null ) ) | ||
}; | ||
} | ||
)( EditorMediaAdvanced ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import { EDITOR_MEDIA_ADVANCED_TOGGLE } from 'state/action-types'; | ||
import { EDITOR_MEDIA_EDIT_ITEM_SET } from 'state/action-types'; | ||
|
||
export function toggleEditorMediaAdvanced() { | ||
export function setEditorMediaEditItem( item ) { | ||
return { | ||
type: EDITOR_MEDIA_ADVANCED_TOGGLE | ||
type: EDITOR_MEDIA_EDIT_ITEM_SET, | ||
item | ||
}; | ||
} |
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 think it works well when we use
noop
from lodash (or any standard noop). I like the idea of being able to quickly and easily grep for these kinds of things, plus if we all use the same one, it's just a single closure/function being generated. obviously this isn't a big dealThere 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, I don't feel strongly either way, though I find it's nice to limit the dependencies, and this should become a common enough pattern for an empty function that I don't think it really harms readability.