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

Update unlink warning message copy #1006

Merged
merged 8 commits into from
Feb 22, 2023
Merged

Conversation

jeffpaul
Copy link
Member

@jeffpaul jeffpaul commented Jan 20, 2023

Description of the Change

The editor notice that displays on remote, distributed posts has some odd spacing (line break where a single space should be, no space after a period) and is missing trailing periods in some instances. This PR attempts to clean up that spacing and missing periods as well as further clarify the terms of "origin" and "remote" posts when referring to distributed content.

See the screenshot from a sample how-to post:

unlink

While I've attempted various searches through the codebase to try and catch all the instances, its possible I missed some so having someone else run a spot-check on this branch to ensure things are all properly updated would be wonderful.

How to test the Change

Ask ChatGPT, I'm curious if it knows.

Changelog Entry

Changed - Descriptive warning message copy on remote, distributed posts.

Credits

Props @jeffpaul, @peterwilsoncc, @cadic.

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@jeffpaul jeffpaul added this to the 2.0.0 milestone Jan 20, 2023
@jeffpaul jeffpaul requested a review from a team as a code owner January 20, 2023 23:26
@jeffpaul jeffpaul self-assigned this Jan 20, 2023
@jeffpaul jeffpaul added the v2 label Jan 31, 2023
Copy link
Collaborator

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one note for now while I figure out what to do with upper case post type labels.

It applies throughout though.

@@ -23,40 +23,40 @@ if (
message = sprintf(
/* translators: 1) Source of content, 2) Distributor post type singular name. */
__(
'Distributed from %1$s. This %2$s is linked to the original. Edits to the original will update this version.',
'Distributed from %1$s. This %2$s is linked to the origin post. Edits to the origin post will update this remote version.',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Distributed from %1$s. This %2$s is linked to the origin post. Edits to the origin post will update this remote version.',
'Distributed from %1$s. This %2$s is linked to the origin %2$s. Edits to the origin %2$s will update this remote version.',

This is so the correct post type is displayed, eg podcasts will read:

Distributed from Distributor Test Site. This Podcast is linked to the origin Podcast. Edits to the origin Podcast will update this remote version.

There is a catch, the dtGutenberg.postTypeSingular uses the case for the singular post name label in register_post_type(), most commonly with an upper case first letter.

We could convert to lowercase before use but I am not sure how this would work with non-english language sites.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm fine with casing being a spot off on this. Are there other places throughout this PR that should similarly be updated @peterwilsoncc?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeffpaul A few more spots, I'll push some commits so I can make sure the string is available in each location.

@peterwilsoncc
Copy link
Collaborator

What I hadn't noticed was that post type strings were cast to lowercase in the PHP but not in the JavaScript so there was an inconsistency already. I've pushed 3fc57c0 to make it consistently lowercase.

@peterwilsoncc
Copy link
Collaborator

I think this is good to go, the strings are consistent across their various uses, the post type names are consistently cased.

Of course, each of the changed lines of code are mine now, so I'll need someone else to review.

@cadic cadic self-requested a review February 22, 2023 09:14
Copy link
Contributor

@cadic cadic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested new messages with the workflow of:

  1. Creating the original
  2. Distributing it
  3. Linking and unlinking

The code is LGTM

I've discovered a new issue #1021 during my tests, was able to reproduce it on the develop branch, so it's not related to the current PR.

@peterwilsoncc peterwilsoncc merged commit 4aeb97e into develop Feb 22, 2023
@peterwilsoncc peterwilsoncc deleted the update/unlink-text branch February 22, 2023 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants