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

Improve post locking modal #10886

Merged
merged 7 commits into from
Nov 19, 2018
Merged

Improve post locking modal #10886

merged 7 commits into from
Nov 19, 2018

Conversation

dimadin
Copy link
Contributor

@dimadin dimadin commented Oct 21, 2018

There are few issues with post locking modal introduced in #4217. This PR tries to fix them and make it look more like post locking modal in classic editor screen (see function _admin_notice_post_locked()).

Current post type

There are two changes here: "All Posts" and "View all posts" texts were changed to use current post type label, and URL of those links now links to the screen of current post type. Both of these changes already exist in post locking modal in classic editor screen.

Translators comments

Explanation that "post" is generic and may be of any type is removed. There are two reasons for this: there are other strings using "post" in this modal that don't have such comment and they shouldn't have it because those strings can be used elsewhere (they would need context instead of comment), and this is how it's done in post locking modal in classic editor screen.

Styling

Link to "All Posts" screen was changed to button for consistency with both other case and usage in post locking modal in classic editor screen.

i18n

Added translator comments for strings with placeholder, added dot at the end of two sentences that didn't have it, and used apostrophe instead of single-quote character as per guidelines.

/* translators: 'post' is generic and may be of any type (post, page, etc.). */
__( '%s now has editing control of this post. Don\'t worry, your changes up to this moment have been saved' ),
/* translators: %s: user's display name */
__( '%s now has editing control of this post. Dont worry, your changes up to this moment have been saved.' ),
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to keep the slashed single quote, the there a reason that was changed here?

Suggested change
__( '%s now has editing control of this post. Dont worry, your changes up to this moment have been saved.' ),
__( '%s now has editing control of this post. Don\'t worry, your changes up to this moment have been saved.' ),

Copy link
Member

Choose a reason for hiding this comment

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

An apostrophe is the correct character to use in English here, not a single quote. It's certainly typographically better; that said, we should just be consistent and use one or the other everywhere. See: #7555.

userDisplayName
) :
/* translators: 'post' is generic and may be of any type (post, page, etc.). */
__( 'Another user now has editing control of this post. Don\'t worry, your changes up to this moment have been saved' )
__( 'Another user now has editing control of this post. Don’t worry, your changes up to this moment have been saved.' )
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
__( 'Another user now has editing control of this post. Dont worry, your changes up to this moment have been saved.' )
__( 'Another user now has editing control of this post. Don\'t worry, your changes up to this moment have been saved.' )

@adamsilverstein
Copy link
Member

@dimadin thank you for the contribution here. overall this looks good, I am not certain about the translator comments - your explanation makes sense.

I left a comment about using escaped single quotes before seeing your link to the guidelines - thanks for that - which are new to me and don't quite match these strings in core and what we have for core standards (https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#strings, https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#single-and-double-quotes) - my only (possibly unfounded) concern would be that these characters would somehow not render correctly in certain fonts or languages.

@dimadin
Copy link
Contributor Author

dimadin commented Nov 12, 2018

As I said in #11710, only few strings use single quote. Unfortunately, GlotPress search doesn't work with , but you can search using code editor and see that all other strings from Gutenberg switched to apostrophe.

my only (possibly unfounded) concern would be that these characters would somehow not render correctly in certain fonts or languages.

Other languages wouldn’t even use those words. 😀 For years I’m using curled quotes „“ when translating strings that have "" instead of “” (for example Hide the teaser before the "More" tag is Сакриј увод пре ознаке „Више“). No problems at all. And there is no reason to have them—we can use any Unicode character, even emoji. 😉

@adamsilverstein
Copy link
Member

@dimadin thanks for clarify, looks good to me!

@adamsilverstein
Copy link
Member

Tested this out locally and looks good. One potential issue using the full post type label for the button is that for longer post type names, the current fixed width design may lead to some button overflow that doesn't look great.

Somewhat of an edge case, this is probably something we can fix by adjusting the CSS in a subsequent PR.

@gziolo
Copy link
Member

gziolo commented Nov 13, 2018

Somewhat of an edge case, this is probably something we can fix by adjusting the CSS in a subsequent PR.

Let's make sure that it's tracked before we merge this PR.

@gziolo gziolo added the Internationalization (i18n) Issues or PRs related to internationalization efforts label Nov 13, 2018
@ocean90
Copy link
Member

ocean90 commented Nov 13, 2018

Unfortunately, GlotPress search doesn't work with

@dimadin It works if you do a case sensitive search as it performs binary comparisons.

@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018
@gziolo gziolo merged commit 56b9553 into WordPress:master Nov 19, 2018
@mtias
Copy link
Member

mtias commented Nov 19, 2018

Thanks for the improvements!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants