i18n rtl css support #900

yen-tt · 2021-07-28T15:23:46Z

use rtl css bundle if locale is a language written from right to left

use rtlcss-webpack-plugin to generate a second CSS bundle that's RTL if there's a locale in locale_config file that requires such bundle.
update html.hbsto update href to bundle.rtl.css and include dir='rtl' if it's a rtl locale
change span styling in verticalResultsCount so its ordering is correct on both ltr and rtl direction

J=SLAP-1458
TEST=manual

launched test site in theme with en, es, and ar locale. See that *.rtl.css bundles are created in public folder. See that en and es pages function normally and ar pages uses rtl format.
launched test site in theme with en and es only. See that no *.rtl.css bundles are created. See that en and es pages function normally.

J=SLAP-1458 TEST=manual

coveralls · 2021-07-28T15:29:55Z

Coverage decreased (-0.02%) to 6.447% when pulling 57129fb on dev/rtl-i18n into 1278c26 on develop.

hbshelpers/cssrtl.js

cea2aj · 2021-07-28T17:00:28Z

static/webpack-config.js

@@ -26,6 +27,19 @@ module.exports = function () {
    });
  }

+  const cssrtlPlugins = [];


nit: I believe this pattern with the array that gets pushed to was created because there can be multiple htmlPlugins. I don't think it's necessary for cssRTL, however it's pretty simple so I'm okay with keeping it as-is if you'd like. Another option would be to make the hasRtlLocale boolean available in this scope, and then after the plugins array is defined we can add the plugin if hasRtrlLocale is true

I see what you mean. I think we should keep this pattern so it's easier to see everything that is included in plugins instead of adding on to it in another chunk of code later in the file. I renamed it to cssRtlPlugin to indicate there's only one if that's clearer.

oshi97 · 2021-07-28T18:57:24Z

static/js/rtl.js

+ * @param {string} locale 
+ * @returns {boolean}
+ */
+module.exports = function isRTL (locale) {


maybe this can go into static/webpack since it's only used as part of the build? right now static/js is for stuff that goes on the browser

static/js/rtl.js

oshi97 · 2021-07-28T19:03:38Z

static/webpack-config.js

@@ -6,6 +6,8 @@ const HtmlPlugin = require('html-webpack-plugin');
 const RemovePlugin = require('remove-files-webpack-plugin');
 const { merge } = require('webpack-merge');
 const { parse } = require('comment-json');
+const RtlCssPlugin = require('rtlcss-webpack-plugin');


just for my edification what kinds of transformations does this do?
does this just auto fix a decent chunk of the sprint items we have?
Maybe a dumb question but I assuuume this is meant to work well with dir="rtl"? Like we won't like double "flip" certain styling properties

it's use, in addition to mini-css-extract-plugin we already have, to create a second rtl css bundle. It uses the rtlcss library under the hood that tom suggested on slack. I'm not sure if it fix most of the sprint items, this might have to apply to SDK css as well. The map error and the chevron change still needs to be fix.

From what I understand, dir="rtl" affects the direction property in CSS for text content flow within a block-level element. But it doesn't change the other styling like margin left/right and whatnots, which is what this plugin would take care of. so I don't think we will double flip things

oshi97 · 2021-07-28T19:06:59Z

static/webpack-config.js

@@ -33,6 +35,19 @@ module.exports = function () {
    globalConfig = parse(globalConfigRaw);
  }

+  const cssRtlPlugin = [];


I agree with Connor using a list for a single item is strange, can't we just have it be

let cssRtlPlugin; //... [ cssRtlPlugin, ...htmlPlugins ]

in the case that we don't need a cssRtlPlugin, it would have an undefined element in there and would throw an error when running webpack. I could move this chunk of code after plugins is first set and add to plugins directly if cssRtlPlugin is required, but then it might not be organize(?) as seeing everything that's suppose to be in plugins to already be there when it's first set.

okay that's fair!

### Features - Support for new languages including Chinese (Traditional), Chinese (Simplified), Russian, Polish, Portuguese, Dutch, Arabic, Korean, Swedish, and Hindi (#918) (#900) - A loading indicator can now be enabled on the search bar (#875) - Voice search support (#894) - Analytics, session tracking, and query source can now be toggled through Runtime Config (#869) (#879) - Added validation to ensure that parts of global_config and page configs are valid (#888) - Font preloads are now located in the layouts/preload-fonts.hbs file (#893) - Global config now includes "sessionTrackingEnabled" and "analyticsEventsEnabled" (#898) - Added a partial for additional Answers configuration (#903) - Map pins now support custom label colors (#922) ### Enhancements - The full-page-map "Search This Area" functionality is better optimized when a user quickly zooms in or out of the map (#861) - Improved build time during development mode (#865) - Icons were updated from components to Handlebars partials to improve performance (#874) ### Bugfixes - When upgrading the theme version, the global config will no longer contain duplicate options which are commented out (#916) - When upgrading the theme, the tests folders are now removed which improves build performance (#913)

#1169) This PR removes styling that targets arbitrary `span`s in `Answers-resultsHeader`. This styling was being automatically applied to icons using the answers-search-ui partial (or other icons) when the `div`s were changed to `span`s for accessibility reasons, which caused visual regressions. This was only an issue on vertical full page map templates which include the search bar and nav tabs in `Answers-resultsHeader`, instead of just the results count and applied filters. Based on the [PR](#900) that initially added this styling, this was needed to ensure results count elements had the correct ordering on both ltr and rtl experiences. I checked a vertical template and vertical full page map template for `en` and `ar` locales in the test site and the results count elements seem to still be flipped, as expected. The only other `span`s that I saw within `Answers-resultsHeader` seemed to be the nav More icon and applied filters, both of which seemed fine after removing this styling. J=TECHOPS-11042 TEST=manual Since we no longer have a paid Percy subscription, our snapshots are not being generated. So I've included some screenshots of pages after the changes in this PR, pointing to v1.16.6 of answers-search-ui which contains the icon `div` to `span` change: **en:** <img width="1008" alt="Screenshot 2024-02-20 at 9 08 26 AM" src="https://github.com/yext/answers-hitchhiker-theme/assets/88398086/a6165616-2316-43b0-9112-7bedf0912017"> <img width="1159" alt="Screenshot 2024-02-20 at 9 08 01 AM" src="https://github.com/yext/answers-hitchhiker-theme/assets/88398086/4338ec87-7c12-4588-842a-7d7836856f11"> <img width="1157" alt="Screenshot 2024-02-20 at 9 07 39 AM" src="https://github.com/yext/answers-hitchhiker-theme/assets/88398086/8296e98e-d853-48b6-ae41-4decdec084cb"> **ar:** <img width="1007" alt="Screenshot 2024-02-20 at 9 09 00 AM" src="https://github.com/yext/answers-hitchhiker-theme/assets/88398086/159e1481-a389-4738-ae9e-ac9544202787"> <img width="1153" alt="Screenshot 2024-02-20 at 9 09 16 AM" src="https://github.com/yext/answers-hitchhiker-theme/assets/88398086/85d7d97b-a3b1-4b0d-9a2f-8752b2225d8e"> <img width="1157" alt="Screenshot 2024-02-20 at 9 09 37 AM" src="https://github.com/yext/answers-hitchhiker-theme/assets/88398086/b550063b-ff37-411b-91b2-840eeb8cc739">

i18n rtl css support

39fe85e

J=SLAP-1458 TEST=manual

yen-tt added the WIP label Jul 28, 2021

fix rtl span order problem

9420c01

yen-tt removed the WIP label Jul 28, 2021

cea2aj reviewed Jul 28, 2021

View reviewed changes

hbshelpers/cssrtl.js Outdated Show resolved Hide resolved

cea2aj reviewed Jul 28, 2021

View reviewed changes

extract rtl check to a function, nits

28ce300

oshi97 reviewed Jul 28, 2021

View reviewed changes

static/js/rtl.js Outdated Show resolved Hide resolved

oshi97 reviewed Jul 28, 2021

View reviewed changes

fix import

57129fb

oshi97 approved these changes Jul 28, 2021

View reviewed changes

yen-tt merged commit 2932efe into develop Jul 28, 2021

yen-tt deleted the dev/rtl-i18n branch July 28, 2021 20:59

cea2aj mentioned this pull request Aug 24, 2021

Version 1.23.0 #933

Merged

nmanu1 mentioned this pull request Feb 20, 2024

Remove styling to allow switching answers-search-ui icon divs to spans #1169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n rtl css support #900

i18n rtl css support #900

yen-tt commented Jul 28, 2021 •

edited

Loading

coveralls commented Jul 28, 2021 •

edited

Loading

cea2aj Jul 28, 2021

yen-tt Jul 28, 2021

oshi97 Jul 28, 2021

oshi97 Jul 28, 2021

yen-tt Jul 28, 2021

yen-tt Jul 28, 2021

oshi97 Jul 28, 2021

oshi97 Jul 28, 2021

yen-tt Jul 28, 2021 •

edited

Loading

oshi97 Jul 28, 2021

i18n rtl css support #900

i18n rtl css support #900

Conversation

yen-tt commented Jul 28, 2021 • edited Loading

coveralls commented Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yen-tt Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yen-tt commented Jul 28, 2021 •

edited

Loading

coveralls commented Jul 28, 2021 •

edited

Loading

yen-tt Jul 28, 2021 •

edited

Loading