Skip to content

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

Merged
kraftbj merged 2 commits intomasterfrom
fix/slideshow-arrows-rtl-7384
Sep 19, 2018
Merged

Slideshows: arrows must point in the right direction for all lang#10163
kraftbj merged 2 commits intomasterfrom
fix/slideshow-arrows-rtl-7384

Conversation

@jeherve
Copy link
Copy Markdown
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 Bug When a feature is broken and / or not performing as intended [Feature] Shortcodes / Embeds [Status] Needs Review This PR is ready for review. [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
Copy Markdown
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
Copy Markdown
Collaborator

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

Generated by 🚫 dangerJS

-webkit-transition: 300ms opacity ease-out;
-moz-transition: 300ms opacity ease-out;
transition: 300ms opacity ease-out;
direction: ltr;
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 This PR is ready for review. 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

Bug When a feature is broken and / or not performing as intended [Feature] Shortcodes / Embeds [Focus] i18n Internationalization / i18n, adaptation to different languages Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants