WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 7 weeks ago

#41994 closed defect (bug) (fixed)

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

Reported by: juhise Owned by: 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 2 months ago.
Patch file
before-patch.png (46.7 KB) - added by juhise 2 months ago.
before-patch
after-patch.png (47.5 KB) - added by juhise 2 months ago.
After patch
41994.diff (1.0 KB) - added by juhise 2 months ago.
Rename patch file name
41994.2.diff (1014 bytes) - added by juhise 2 months ago.
Update patch
no-gallery-selected.png (20.3 KB) - added by juhise 2 months ago.
no gallery selected
41994.3.diff (946 bytes) - added by Presskopp 7 weeks ago.
41994.4.diff (1.6 KB) - added by helen 7 weeks ago.
41994.5.diff (1.8 KB) - added by Presskopp 7 weeks ago.
41994-patched.jpg (48.3 KB) - added by Presskopp 7 weeks ago.

Download all attachments as: .zip

Change History (39)

@juhise
2 months ago

Patch file

@juhise
2 months ago

before-patch

@juhise
2 months ago

After patch

@juhise
2 months ago

Rename patch file name

#1 @swissspidy
2 months ago

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

#2 @juhise
2 months 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
2 months ago

Update patch

@juhise
2 months ago

no gallery selected

#3 follow-up: @melchoyce
2 months 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.


2 months ago

#5 in reply to: ↑ 3 @joemcgill
2 months 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
2 months ago

@melchoyce @joemcgill

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

#7 @melchoyce
2 months 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
2 months 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.


8 weeks ago

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


8 weeks ago

#11 @melchoyce
8 weeks 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
8 weeks ago

  • Type changed from enhancement to defect (bug)

#13 follow-up: @juhise
8 weeks 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
7 weeks 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.


7 weeks ago

#16 @Presskopp
7 weeks 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 7 weeks ago by Presskopp (previous) (diff)

#18 @Presskopp
7 weeks 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
7 weeks ago

@helen
7 weeks ago

#19 @helen
7 weeks ago

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

#20 @melchoyce
7 weeks ago

Tested, looks good 👍

#21 @Presskopp
7 weeks ago

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

#22 follow-up: @westonruter
7 weeks ago

@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?

Last edited 7 weeks ago by westonruter (previous) (diff)

#23 @westonruter
7 weeks 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
7 weeks 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 7 weeks ago by westonruter (previous) (diff)

#25 in reply to: ↑ 22 @helen
7 weeks 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
7 weeks ago

#26 @Presskopp
7 weeks ago

  • Keywords has-patch added; needs-patch removed

#27 @Presskopp
7 weeks 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.


7 weeks ago

#29 @westonruter
7 weeks 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.