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.

Sign up for GitHub

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

Jump to bottom

Likes: Bust iframe cache to facilitate string fix #8228

Merged
merged 1 commit into from
Nov 27, 2017
Merged

Likes: Bust iframe cache to facilitate string fix #8228

merged 1 commit into from
Nov 27, 2017

Conversation

mhsdef
Copy link

@mhsdef mhsdef commented Nov 22, 2017

We need to pull down new translations for comment likes.

Fixes #7958

Changes proposed in this Pull Request:

From the backend [when cache is busted]:

  • New comment like strings with better context for translators.
  • Denesting of some ifs in likes-rest.js.

Testing instructions:

  • Apply D8423-code
  • Point widgets.wp.com to your SB.
  • Clone this to your test site.
  • Use a language that has translations for the new comment like strings.

Proposed changelog entry for your changes:

Fixed comment likes localization bugs.

@mhsdef mhsdef added [Feature] Comment Likes [Status] Needs Review This PR is ready for review. [Type] Bug When a feature is broken and / or not performing as intended labels Nov 22, 2017
@mhsdef mhsdef requested review from jeherve and vindl November 22, 2017 06:12
@mhsdef mhsdef requested a review from a team as a code owner November 22, 2017 06:12
jeherve
jeherve approved these changes Nov 22, 2017
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Thanks!

Let's merge this as soon as D8423-code gets committed.

@jeherve jeherve added [Focus] i18n Internationalization / i18n, adaptation to different languages [Status] Ready to Merge Go ahead, you can push that green button! [Pri] High and removed [Status] Needs Review This PR is ready for review. labels Nov 22, 2017
vindl
vindl approved these changes Nov 22, 2017
View reviewed changes
Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @hewsut!

vindl
vindl reviewed Nov 23, 2017
View reviewed changes
modules/likes/jetpack-likes-master-iframe.php Outdated
@@ -4,7 +4,7 @@
* This function needs to get loaded after the like scripts get added to the page.
*/
function jetpack_likes_master_iframe() {
$version = '20170629';
$version = '20171122v2';
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember running into v2 appended to date in rest of the codebase. Is there some specific reason for this, and could we just use 20171123 instead?

Copy link
Member

@vindl vindl Nov 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Actually I think we can also use JETPACK__VERSION here to avoid the need to bump this manually in the future. Not sure if it would cause issues for Post Likes though. /cc @pento

Copy link
Author

Choose a reason for hiding this comment

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

could we just use 20171123 instead?

At this point, sure, we're onto that date. I did v2 as I seemed to cache a bad variant I tested with under 20171122.

@vindl
Copy link
Member

vindl commented Nov 23, 2017

One thing I noticed while testing is that with new translations some longer strings are cropped due to fixed iframe width. This didn't occur with previous English translations, but could likely look broken in some other languages already.

oneotherperson

We can work around this by changing the width here to 100% so the iframe assumes the width of parent container (in https://github.com/Automattic/jetpack/blob/master/modules/likes/queuehandler.js#L305). I can also spin up a separate PR for this if you'd like.

@vindl vindl added this to the 5.6 milestone Nov 23, 2017
@mhsdef
Bust Likes iframe cache to facilitate string fix
e1371b6 
We need to pull down new translations for comment likes.
@mhsdef mhsdef force-pushed the fix/comment-likes-localization branch from dc1753e to e1371b6 Compare November 27, 2017 03:22
@mhsdef
Copy link
Author

mhsdef commented Nov 27, 2017

I can also spin up a separate PR for this if you'd like.

I am a little confused so, sounds good, let's do that if you would. I didn't see the issue on my test site:

screen shot 2017-11-26 at 10 37 25 pm

@mhsdef mhsdef merged commit 5351f61 into master Nov 27, 2017
@mhsdef mhsdef deleted the fix/comment-likes-localization branch November 27, 2017 03:53
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 27, 2017
@vindl
Copy link
Member

vindl commented Nov 27, 2017

I am a little confused so, sounds good, let's do that if you would. I didn't see the issue on my test site:

Will do it. 👍 It only occurs when you have one instead of some one digit number (and person instead of people) due to higher number of characters.

@vindl vindl mentioned this pull request Nov 27, 2017
Merged
jeherve added a commit that referenced this pull request Nov 28, 2017
@jeherve
Changelog: add #8228
e9fcccd
oskosk pushed a commit that referenced this pull request Nov 28, 2017
@jeherve @oskosk
Changelog 5.6: running changelog. (#8192)
7844644 
* Changelog 5.6: create base for changelog.

* Update changelog with 5.5.1 info.

* Changelog: add #7930 and #8238

* Changelog: add #8076

* Changelog: add #8100

* Changelog: add #8117

* Changelog: add #8141

* Changelog: add #8143

* Changelog: add #8147

* Changelog: add #8149

* Changelog: add #8153

* Changelog: add #8173

* Changelog: add #8184

* Changelog: add #8196

* Changelog: add #8199

* Changelog: add #8093

* Changelog: add #8171

* Changelog: add #8182

* Changelog: add #8202, #8222

* Changelog: add #8228

* Changelog: add #8240

* Changelog: add #8251

* remove AL card change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vindl vindl vindl approved these changes

@jeherve jeherve jeherve approved these changes

Assignees
No one assigned
Labels
[Feature] Comment Likes [Focus] i18n Internationalization / i18n, adaptation to different languages [Pri] High [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
5.6
Development

Successfully merging this pull request may close these issues.
4 participants
@mhsdef @vindl @jeherve @matticbot