Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#41994 closed defect (bug) (fixed)

New Gallery Widget: Add Media button label should be updated for consistency

Reported by: juhise's profile juhise Owned by: westonruter's profile westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Widgets Keywords: has-screenshots good-first-bug has-patch
Focuses: ui Cc:

Description

I noticed in previous Media widgets Button: label is Add Image, Add Video but in gallery widget Select Images is being used. I think it should be Add Images

Attachments (10)

41993.diff (1.0 KB) - added by juhise 6 years ago.
Patch file
before-patch.png (46.7 KB) - added by juhise 6 years ago.
before-patch
after-patch.png (47.5 KB) - added by juhise 6 years ago.
After patch
41994.diff (1.0 KB) - added by juhise 6 years ago.
Rename patch file name
41994.2.diff (1014 bytes) - added by juhise 6 years ago.
Update patch
no-gallery-selected.png (20.3 KB) - added by juhise 6 years ago.
no gallery selected
41994.3.diff (946 bytes) - added by Presskopp 6 years ago.
41994.4.diff (1.6 KB) - added by helen 6 years ago.
41994.5.diff (1.8 KB) - added by Presskopp 6 years ago.
41994-patched.jpg (48.3 KB) - added by Presskopp 6 years ago.

Download all attachments as: .zip

Change History (39)

@juhise
6 years ago

Patch file

@juhise
6 years ago

before-patch

@juhise
6 years ago

After patch

@juhise
6 years ago

Rename patch file name

#1 @swissspidy
6 years ago

  • Keywords has-patch has-screenshots added
  • Milestone changed from Awaiting Review to 4.9

#2 @juhise
6 years ago

I just had a look again and I felt Add Gallery would look better and when no media is selected label should be No gallery selected

@juhise
6 years ago

Update patch

@juhise
6 years ago

no gallery selected

#3 follow-up: @melchoyce
6 years ago

"Add Gallery / No gallery selected" feels a little bit like it implies galleries are a unit of images you can select, so I'm personally leaning more towards "Add images / No images selected" as a better solution.

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.


6 years ago

#5 in reply to: ↑ 3 @joemcgill
6 years ago

Replying to melchoyce:

"Add Gallery / No gallery selected" feels a little bit like it implies galleries are a unit of images you can select, so I'm personally leaning more towards "Add images / No images selected" as a better solution.

I love this improvement, but had the same reaction as Mel and would also prefer the "Add images / No images selected".

#6 @juhise
6 years ago

@melchoyce @joemcgill

We are going to add unit of images (Gallery) only and everywhere else we have Edit Gallery, Replace Gallery.

#7 @melchoyce
6 years ago

"Edit Gallery" makes sense to me, because at this point you have created a gallery as a unit of content. You're then editing the contents of the gallery, rather than editing the images themselves, and replacing the gallery with an entirely new set of images, rather than replacing individual images. (Though, I wonder if "Replace" is even necessary, since "Edit" is functionally the same in this particular case?)

#8 @juhise
6 years ago

Yeah agree Edit Gallery make sense to me too. Replace gallery gives the option to create a new gallery and add it to widget. Thought not sure what would be the real case scenario that anyone would need to create new gallery rather then editing the existing one.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.


6 years ago

#11 @melchoyce
6 years ago

Chatted with @westonruter and our final decision is:

  • [ Add Images ] before picking images
  • [ Edit Gallery ] after picking images
  • Remove [ Replace Gallery ]

@juhise Do you want to make another patch?

#12 @melchoyce
6 years ago

  • Type changed from enhancement to defect (bug)

#13 follow-up: @juhise
6 years ago

@melchoyce

That sounds great, we already have patch for Add Images. I don't think I would like to add anything else.

#14 in reply to: ↑ 13 @melchoyce
6 years ago

  • Keywords good-first-bug needs-patch added; has-patch removed

Replying to juhise:

@melchoyce

That sounds great, we already have patch for Add Images. I don't think I would like to add anything else.

Okay, thank you for your contributions :)

Would anyone else like to pick this up and work from @juhise's patch? Would love to merge it tomorrow.

This ticket was mentioned in Slack in #core by melchoyce. View the logs.


6 years ago

#16 @Presskopp
6 years ago

@melchoyce @westonruter I'd love to attach a patch, but I'm not sure how to "Remove [ Replace Gallery ]". This button gets added in class-wp-widget-media.php for any media widget.

Last edited 6 years ago by Presskopp (previous) (diff)

#18 @Presskopp
6 years ago

Thanks, yes, I see the strings there, but obviously I still don't get the point. What is the desired result regarding the 'replace' button ?

Attached a patch, though, let me know if there's more to change (or just do it yourself)

@Presskopp
6 years ago

@helen
6 years ago

#19 @helen
6 years ago

I think the idea is to just remove the replace button, attached 41994.4.diff for the sake of time :)

#20 @melchoyce
6 years ago

Tested, looks good 👍

#21 @Presskopp
6 years ago

thanks @helen, I didn't dare to just cut it out.

#22 follow-up: @westonruter
6 years ago

@helen @helen That removes the Replace button from all media widgets, so we can't do that. It would mean you could never change the an image/video/audio after you selected it the first time, right?

Version 0, edited 6 years ago by westonruter (next)

#23 @westonruter
6 years ago

I think a better approach would be to just hide the .change-media button in the Gallery widget via CSS. Or we could wrap the button in the template with a conditional:

<# if ( ! data.hideReplaceButton ) { #>
	<button type="button" class="button change-media select-media selected">
		<?php echo esc_html( $this->l10n['replace_media'] ); ?>
	</button>
<# } #>

And then introduce this hideReplaceButton prop in the Gallery widget only.

#24 @westonruter
6 years ago

Or, alternatively, this condition could be based on whether or not $this->l10n['replace_media'] is populated to begin with in PHP. So then WP_Media_Widget_Gallery could define replace_media as null and then the template could do:

<?php if ( ! empty( $this->l10n['replace_media'] ) ) : ?>
        <button type="button" class="button change-media select-media selected">
                <?php echo esc_html( $this->l10n['replace_media'] ); ?>
        </button>
<?php endif; ?>
Last edited 6 years ago by westonruter (previous) (diff)

#25 in reply to: ↑ 22 @helen
6 years ago

Replying to westonruter:

@helen That removes the Replace button from all media widgets, so we can't do that. It would mean you could never change the an image/video/audio after you selected it the first time, right?

Ah yes, I forgot about the separate edit details part.

Replying to westonruter:

Or, alternatively, this condition could be based on whether or not $this->l10n['replace_media'] is populated to begin with in PHP.

Should it check in PHP or in the JS template? I guess that's a matter of semantics and maybe ever so slightly performance?

@Presskopp
6 years ago

#26 @Presskopp
6 years ago

  • Keywords has-patch added; needs-patch removed

#27 @Presskopp
6 years ago

@westonruter patch attached like proposed in https://core.trac.wordpress.org/ticket/41994#comment:24

This ticket was mentioned in Slack in #core by presskopp. View the logs.


6 years ago

#29 @westonruter
6 years ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 41772:

Widgets: Remove "Replace Gallery" button from Gallery widget since redundant with "Edit Gallery".

Props Presskopp, juhise, melchoyce.
See #41914.
Fixes #41994.

Note: See TracTickets for help on using tickets.