Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#50773 reviewing defect (bug)

Merge duplicate strings in bulk actions and row actions for list tables

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: I18N Keywords: has-patch needs-testing needs-testing-info needs-refresh
Focuses: ui, administration Cc:

Description

Backround: #40244, #50747.

As noted by @ramiy in comment:25:ticket:40244 and me in comment:5:ticket:50747, as part of the changes in [48352] and [48595] some strings were duplicated with a different case:

  • _x( 'Not Spam', 'comment' )
  • _x( 'Not spam', 'comment' )
  • _x( 'Not Spam', 'site' )
  • _x( 'Not spam', 'site' )

Additionally, some strings kept title casing and should be switched to sentence casing for consistency with others:

  • __( 'Enable Auto-updates' )
  • __( 'Disable Auto-updates' )
  • __( 'Network Activate' )
  • __( 'Network Deactivate' )
  • __( 'Network Enable' )
  • __( 'Network Disable' )

The problem is that some of these strings are not only used in bulk actions, but also in row actions.

While most of bulk action strings appear to be switched to sentence casing across the admin now, row actions are not. It seems like that would be the next step to bring some consistency here.

Attachments (5)

50773.diff (35.4 KB) - added by dkotter 4 years ago.
50773.2.patch (36.2 KB) - added by justinahinon 4 years ago.
50773.3.patch (961 bytes) - added by justinahinon 4 years ago.
Fix strings cases inconsistencies in JS files
50773.4.diff (37.1 KB) - added by dkotter 4 years ago.
Combine patches 50773.2 and 50773.3 together
50773.2.diff (95.2 KB) - added by justinahinon 4 years ago.

Download all attachments as: .zip

Change History (36)

#1 in reply to: ↑ description @ramiy
4 years ago

Replying to SergeyBiryukov:

While most of bulk action strings appear to be switched to sentence casing across the admin now, row actions are not. It seems like that would be the next step to bring some consistency here.

I agree, the next logical step is to use sentence casing in "Row Actions".

@dkotter
4 years ago

#2 @dkotter
4 years ago

I've gone through all of the list-table classes and updated strings to use sentence case, where they were previously using title case.

This should take care of the "Row Actions" but might impact other areas as well. If the goal is to tackle those strings in a different ticket, I can clean up my patch to focus only on the "Row Actions". Just seemed to make the most sense to fix all of those at once.

#3 @garrett-eclipse
4 years ago

  • Keywords has-patch needs-testing added

#4 follow-up: @ramiy
4 years ago

Does the patch include translation strings from #50747 ?

#5 in reply to: ↑ 4 @dkotter
4 years ago

Replying to ramiy:

Does the patch include translation strings from #50747 ?

Yes, this contains those updates

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


4 years ago

#7 @helen
4 years ago

I see some changes on the latest patch in #50747 that aren't in this patch - could we perhaps pick one ticket and merge patches together? And a review for consistency across these UIs would be nice :)

#8 @justinahinon
4 years ago

I'll go through both and update the patch.

#9 @justinahinon
4 years ago

I've merged https://core.trac.wordpress.org/attachment/ticket/50773/50773.diff with https://core.trac.wordpress.org/attachment/ticket/50747/50747.2.patch.

However, there are others occurences of this inconsistencies in js files. I have an upcoming patch tomorrow.

@justinahinon
4 years ago

Fix strings cases inconsistencies in JS files

#10 @garrett-eclipse
4 years ago

  • Keywords needs-refresh added

@justinahinon it appears the latest 50773.3.patch only contains the JS strings and is missing all the changes found in 50773.2.patch. Do you have time to merge these so we have a single patch to review/test?

@dkotter
4 years ago

Combine patches 50773.2 and 50773.3 together

#11 @dkotter
4 years ago

@garrett-eclipse @justinahinon I noticed the same while testing this ticket earlier today. I've gone ahead and combined those two patches together into a new patch.

#12 @JeffPaul
4 years ago

  • Keywords needs-refresh removed

#13 @hellofromTonya
4 years ago

#50747 is a duplicate of this ticket. Per this scrub, this ticket is the commit candidate.

Note to committer:
When this ticket is committed, please add props from ticket #50747.

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


4 years ago

@justinahinon
4 years ago

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


4 years ago

#16 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#17 @audrasjb
4 years ago

  • Keywords needs-testing removed

Thanks @justinahinon 🙌 patch looks ready now, marking this for commit as per today's bug scrub

#18 @hellofromTonya
4 years ago

  • Keywords commit added

#19 @SergeyBiryukov
4 years ago

  • Keywords commit removed
  • Milestone changed from 5.6 to 5.7

Somes notes on 50773.2.diff:

  • With the capitalization removed, some of the strings would benefit from quotes. Otherwise, in sentences like "...and click Install now when you are prompted in the popup window", it's not quite clear that the name of the button is "Install now" and not "Install". Or in "click the Upload plugin button", it's not clear that the name is "Upload Plugin" and not "Upload".
  • Looking at the change in src/wp-admin/includes/file.php, the new "Search results" string is inconsistent with "Posts Page", "Date Template", etc. The other strings will likely need a change too.
  • “%s” (Edit) should probably be left as is.

Would like to take some more time to review this carefully. Moving to the next release for now.

#20 @SergeyBiryukov
4 years ago

#50747 was marked as a duplicate.

#21 @lukecarbis
4 years ago

  • Milestone changed from 5.7 to 5.8

Since we're in 5.7 beta 3, let's move this to the next release again.

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


4 years ago

#23 @chaion07
4 years ago

We reviewed this ticket during a recent [bug-scrub session]https://wordpress.slack.com/archives/C02RQBWTW/p1622579544172100. Pinging @SergeyBiryukov to see if you have been able to review this more closely.Really hoping for a feedback for this by Beta 1(June 8) so that a patch refresh can be obtained. Otherwise it is likely that this ticket is getting punted to Future Release. Thanks!

Props to @jeffpaul https://wordpress.slack.com/archives/C02RQBWTW/p1622579639172900

#25 @justinahinon
4 years ago

Hi @SergeyBiryukov
I've just sent this PR https://github.com/WordPress/wordpress-develop/pull/1340 with your last suggestions.

#26 @hellofromTonya
4 years ago

  • Keywords needs-testing needs-testing-info added
  • Milestone changed from 5.8 to 5.9

Today is 5.8 Beta 1. Thank you @justinahinon for submitting the PR. Unfortunately, ran out of time to review and test. Punting to 5.9 where work can continue.

#27 @hellofromTonya
3 years ago

  • Milestone changed from 5.9 to 6.0

Given the inactivity and 5.9 Beta 1 is in 4 days, moving to 6.0.

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


3 years ago

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


3 years ago

#30 @Toro_Unit
3 years ago

We reviewed this ticket during a recent [bug-scrub session](https://wordpress.slack.com/archives/C02RQBWTW/p1648791507136399)

@SergeyBiryukov Interested in addressing this ticket in 6.0?

Also, can anyone review these patches?

#31 @costdev
3 years ago

  • Keywords needs-refresh added
  • Milestone changed from 6.0 to Future Release

As we're in soft string freeze for 6.0, the PR needs a refresh and this has been moved for several milestones, I'm moving this to Future Release until it's ready to be milestoned.

Note: See TracTickets for help on using tickets.