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

Slideshows: arrows must point in the right direction for all lang #10163

Merged
merged 2 commits into from
Sep 19, 2018

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Sep 18, 2018

Fixes #7384

Changes proposed in this Pull Request:

Explicitly adding a direction to the default css ensures that direction gets flipped in the automatically generated RTL version of the file.

Testing instructions:

  1. Add a slideshow to one of your posts (add a gallery, choose the slideshow gallery type).
  2. In Settings > General, switch your site language to Hebrew for example.
  3. View the post.
  4. Ensure that the arrows in the slideshow point away from the center:

They should not look like this:

Note that the RTL file must be generated first, so you'll need to run yarn build, and only after #10162 has been merged.

Proposed changelog entry for your changes:

Slideshows: ensure arrows point in the right direction for RTL Languages.

Fixes #7384

Explicitely adding a direction to the default css ensures that direction gets flipped in the automatically generated RTL version of the file.
@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Shortcodes / Embeds [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Focus] i18n Internationalization / i18n, adaptation to different languages labels Sep 18, 2018
@jeherve jeherve added this to the 6.6 milestone Sep 18, 2018
@jeherve jeherve self-assigned this Sep 18, 2018
@jeherve jeherve requested a review from a team as a code owner September 18, 2018 15:45
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D18463-code. (newly created revision)

@jetpackbot
Copy link

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

@@ -99,6 +99,7 @@ body div.slideshow-window * img {
-webkit-transition: 300ms opacity ease-out;
-moz-transition: 300ms opacity ease-out;
transition: 300ms opacity ease-out;
direction: ltr;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like direction was already here, in the line 98:

direction:ltr;
-webkit-transition: 300ms opacity ease-out;
-moz-transition: 300ms opacity ease-out;
transition: 300ms opacity ease-out;
direction: ltr;

There's /* @noflip */ just above this CSS class, does that have something to do with original issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh heh, I need new glasses I guess.:) Thanks for taking a look!

There's /* @NoFlip */ just above this CSS class, does that have something to do with original issue?

That /* @noflip */ rule does not appear to be respected (direction gets flipped in the RTL file anyway), so that seems to be the culprit here indeed.

I pushed a new commit with the correct notation (/*rtl:ignore*/) for the library we use now, and it seems to work! I believe I got tricked by a cached file earlier.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D18463-code. (updated diff)

@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 19, 2018
@kraftbj kraftbj merged commit 3c8f25f into master Sep 19, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 19, 2018
@kraftbj kraftbj deleted the fix/slideshow-arrows-rtl-7384 branch September 19, 2018 21:26
jeherve added a commit that referenced this pull request Sep 24, 2018
jeherve added a commit that referenced this pull request Sep 25, 2018
* Readme: add boilerplate for next release, 6.6

* Add 6.5 to the changelog.txt file

* Set boilerplate testing list for 6.6

* Readme: update stable tag to 6.5

* Add bullets to 6.5 changelog items

* Readme: add link to previous changelogs

This will help folks who want to know more about past releases,
while keeping the readme.txt short so as to not overwhelm translators and site owners only looking for information about the last release.

* Changelog: add information at the top of the changelog file.

* Changelog: add #10054

* Changelog: add #10078

* Changelog: add #10079

* Changelog: add #10064

* Changelog: add #10094

* Changelog: add #10096

* Testing list: add more information based on #10087

* Changelog: add #9847

* Changelog: add #10084

* Changelog: add #9918

* Changelog: add #7614

* Changelog: add #10116

* Changelog: add #10108

* Changelog: add #10041

* Changelog: add #10121

* Changelog: add #10134

* Changelog: add #10130

* Changelog: add #10109

* changelog: add #10137

* changelog: add #9952

* changelog: add #10120

* changelog: add #10162

* Changelog: add #10163

* Changelog: add #10092

* changelog: add #10156

* Changelog: add #10154

* changelog: add #10122

* Changelog: add #10101

* changelog: add #10105

* changelog: add #10190

* Changelog: add #10196

* changelog: add #10152

* Changelog: add #10153

* Testing list: add more details to Site Verification testing steps.

@see #10143 (comment)

* changelog: add #10194

* Changelog: add #10193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Shortcodes / Embeds [Focus] i18n Internationalization / i18n, adaptation to different languages Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants