Skip to content
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: Implement WIP advanced image settings dialog #2085

Merged
merged 1 commit into from
Jan 14, 2016
Merged

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Jan 4, 2016

Related: #307

This pull request seeks to implement a minimal set of behavior related to advanced image settings editing. Specifically, the changes included add a new TinyMCE toolbar button to be shown in the inline toolbar for images which, when clicked, toggles the display of a <Dialog /> component, currently rendered with dummy content.

wip

Implementation notes:

I fully expect the structure of the Redux state and <EditorMediaAdvanced /> component to need extensive revisions as this functionality is built, thus the lack of proper documentation and testing. That said, feel free to comment on the proposed structure as-is.

Testing instructions:

Confirm that the included behavior is only shown in development environments by temporarily setting post-editor/media-advanced to false in config/development.json and restarting make run.

  1. Navigate to the Calypso post editor
  2. Select a site, if prompted
  3. Insert an image to the post content
  4. Click the image to select it
  5. Note that...
    • If post-editor/media-advanced is true, an "Edit" icon is shown in the inline toolbar
    • If post-editor/media-advanced is false, an "Edit" icon is not shown in the inline toolbar and there are no errors in the developer tools console
  6. Click the Edit button in the inline toolbar
  7. Note that a dialog is shown with dummy content

@aduth aduth added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Media The media screen in Calypso, general media management, or integration with third party media. labels Jan 4, 2016
@aduth aduth self-assigned this Jan 4, 2016
EditorMediaAdvanced.defaultProps = {
visible: false,
toggleVisible: () => {}
};
Copy link
Member

Choose a reason for hiding this comment

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

why put these here when we are already using destructuring in the function declaration?

function EditorMediaAdvanced( { visible = false, toggleVisible = noop } ) {
    
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why put these here when we are already using destructuring in the function declaration?

You know, that's an interesting question. One has to wonder why defaultProps was one of the two properties that the React team chose to support for stateless functional components. Perhaps there's some semantics of defaultProps that wouldn't be reflected with the destructured default values? I couldn't find much discussion on the topic, though this commenter appears to prefer default arguments from a function purity perspective. On the flip-side, it does make the function declaration line a bit longer.

Do you have a strong preference either way? I'm inclined to keep it as is, mostly for aesthetics to this last point and for proximity to the propTypes declaration for quick scanning. Additionally, it may be that this is easier for static analysis if we choose to pursue some automated documentation of components.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have a strong preference either way?

not at all 😄

@kellychoffman
Copy link
Member

If post-editor/media-advanced is true, an "Edit" icon is shown in the inline toolbar

👍

If post-editor/media-advanced is false, an "Edit" icon is not shown in the inline toolbar and there are no errors in the developer tools console

👍

@aduth aduth force-pushed the add/media-advanced branch from e35c6dd to 493e88b Compare January 14, 2016 21:03
@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 14, 2016
aduth added a commit that referenced this pull request Jan 14, 2016
Media: Implement WIP advanced image settings dialog
@aduth aduth merged commit 9a54050 into master Jan 14, 2016
@aduth aduth deleted the add/media-advanced branch January 14, 2016 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants