-
Notifications
You must be signed in to change notification settings - Fork 2k
Media: Correctly import EditorMediaModalDetail #18705
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
Conversation
Test live: https://calypso.live/?branch=fix/media-modal |
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.
👍 🚢
@@ -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'; |
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.
Can the export const
be removed from this file as well?
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.
Hmm, good point. I was foolishly assuming it was being exported separately for testing, but that assumption doesn't seem to hold...
It fixed the problem 👍 |
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).
Fixes a bug I introduced with #18590, as reported by @jorgefilipecosta in Slack:
Turns out that
EditorMediaModalDetail
is properlylocalize()
d, butmy-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.