Skip to content

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Oct 10, 2017

Fixes a bug I introduced with #18590, as reported by @jorgefilipecosta in Slack:

If open media and select an image and then press edit I get an error related with localize it may be related with the last changes

image

Turns out that EditorMediaModalDetail is properly localize()d, but my-sites/media/main was importing the unlocalized version of the component.

To test: Verify that the above error is gone, and that everything else works as before.

@ockham ockham added [Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Pri] High Address as soon as possible after BLOCKER issues [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 10, 2017
@ockham ockham self-assigned this Oct 10, 2017
@matticbot
Copy link
Contributor

Copy link
Contributor

@jsnmoon jsnmoon left a comment

Choose a reason for hiding this comment

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

👍 🚢

@ockham ockham merged commit 266ed40 into master Oct 10, 2017
@ockham ockham deleted the fix/media-modal branch October 10, 2017 17:47
@@ -15,7 +15,7 @@ import { localize } from 'i18n-calypso';
import MediaLibrary from 'my-sites/media-library';
import SidebarNavigation from 'my-sites/sidebar-navigation';
import Dialog from 'components/dialog';
import { EditorMediaModalDetail } from 'post-editor/media-modal/detail';
import EditorMediaModalDetail from 'post-editor/media-modal/detail';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the export const be removed from this file as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good point. I was foolishly assuming it was being exported separately for testing, but that assumption doesn't seem to hold...

@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 10, 2017
@jorgefilipecosta
Copy link
Member

It fixed the problem 👍

ockham added a commit that referenced this pull request Oct 11, 2017
Re-introduce the un-`connect()`ed named export of `EditorMediaModalDetail` again, and use that in `media/main`.

Rationale: Quoting a comment I'm adding to `post-editor/media-modal/detail/index.jsx`:

> The default export is only used by the post editor, which displays the image or
> video editor depending on Redux state, which is set by the actions below.
> In the Media library (i.e. `/media`) OTOH, we're explicitly passing `onEditImageItem`
> and `onEditVideoItem` as props to the _named_ export (above), and use them to set
> component state there to conditionally display the image/video editor.
> (This is also the reason why we're `localize()`ing the named export.)
> TODO: Fix this mess, rely on Redux state everywhere.

Fixes #18727, which was caused by #18705 (tangential: #18707).
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. [Pri] High Address as soon as possible after BLOCKER issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants