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

Add "Add Post Element" to media buttons #551

Merged
merged 5 commits into from
Nov 8, 2016
Merged

Conversation

goldenapples
Copy link
Contributor

Includes an "Add Post Element" to the media buttons array, which pops
open the Insert Post Element frame of the media modal.

See #131

props to @ninetienne and @Vaince for code snippets

Includes an "Add Post Element" to the media buttons array, which pops
open the Insert Post Element frame of the media modal.

props @ninetienne and @Vaince for code snippets in
#131
@goldenapples
Copy link
Contributor Author

Clicking this button:

screen shot 2015-11-18 at 1 51 07 pm

will bring up the "Insert Post Element" state of the media modal.

Curious to hear feedback on this. I know there were concerns raised about adding more buttons to the editor, but on the other hand, this has been a very popular request. We have time before the next release to put some thought into the UI, so it would be good to get some community input. I think adding a media button but not an MCE button is a good middle ground between discoverability and clutter.

Fix indentation of multi-line function, use strict mode for `in_array()`
comparison...
@danielbachhuber
Copy link
Contributor

Curious to hear feedback on this.

I dig it. 👍 from me.

@mattheu
Copy link
Contributor

mattheu commented Feb 29, 2016

I like the button. But I'm not so sure about the UX of having 2 links to the same modal. I think we need to go further and split this out from the existing media modal completely. See #582

@khromov
Copy link
Contributor

khromov commented Feb 29, 2016

Putting this in the media dialogue has always felt a bit awkward, and we've gotten feedback from multiple editors that it's a useless extra click, so +1 this!

@0aveRyan
Copy link

Small issue I've encountered, still working on a fix -- The media modal retains its' state on close.

The script works initially to launch into the right view, but if an image or other media element is inserted first, the modal retains state and will return to image, gallery, etc. Apparently WordPress Core intends to retain state (see: https://core.trac.wordpress.org/ticket/27480). I've spent a few minutes attempting to tap into the modal and reset any existing states and force shortcode-ui, but haven't been successful yet.

@0aveRyan
Copy link

@goldenapples right before the wp.media.editor.open( editor, options ), if you add this:

wp.media.editor.remove( editor );

The modal will always trigger the right state, even if there's a stored state from Inserting an image or other view.

Ensure that the editor will open to the correct state, even if there's a
stored global state from inserting an image or another action. Props
@0aveRyan.
@goldenapples
Copy link
Contributor Author

Thanks @0aveRyan - that seems to work perfectly for triggering the correct state of the modal. I think we'll also need to find an appropriate place of resetting the modal state when the shortcode-ui modal is closed; otherwise, after once clicking "Add post element", when the user tries to add media, they'll get the "Insert Post Element" state.

@0aveRyan
Copy link

0aveRyan commented Sep 13, 2016

@goldenapples that would require a more invasive change to the callback where Shortcake inserts the shortcode or the close method is triggered.

I agree and personally prefer the state to reset on close / insertion as well... but the discussion on that WP Core ticket indicates they prefer the state is retained, so while it definitely makes sense to unset for this button, think we should get feedback from someone on wiping out state elsewhere before exploring that.

Updates php-lint and wpcs version so that build can pass again.
@goldenapples
Copy link
Contributor Author

I merged in master to fix the build lint error; this is ready for a review/merge now.

@mattheu
Copy link
Contributor

mattheu commented Nov 8, 2016

Looks good. Tested and works well.

@mattheu mattheu merged commit 0adfdd2 into master Nov 8, 2016
@mattheu mattheu deleted the 131-add-post-element-button branch November 8, 2016 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants