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

i18n rtl css support #900

Merged
merged 4 commits into from
Jul 28, 2021
Merged

i18n rtl css support #900

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39fe85e
i18n rtl css support
Jul 28, 2021
9420c01
fix rtl span order problem
Jul 28, 2021
28ce300
extract rtl check to a function, nits
Jul 28, 2021
57129fb
fix import
Jul 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions hbshelpers/isRTL.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
const checkRTL = require('../static/js/rtl');

/**
* Returns true if the given language is written from right to left (requires rtl css)
*
* @param {string} locale
* @returns {boolean}
*/
module.exports = function isRTL(locale) {
return checkRTL(locale);
}
10 changes: 8 additions & 2 deletions layouts/html.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,13 @@
</script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/iframe-resizer/4.2.10/iframeResizer.contentWindow.min.js"></script>
<link rel="stylesheet" type="text/css" href="{{sdkAssetUrl global_config.sdkVersion 'en' 'answers.css'}}">
<link rel="stylesheet" type="text/css" href="{{relativePath}}/bundle.css" data-webpack-inline>
<link rel="stylesheet" type="text/css"
{{#if (isRTL global_config.locale) }}
href="{{relativePath}}/bundle.rtl.css"
{{else}}
href="{{relativePath}}/bundle.css"
{{/if}}
data-webpack-inline>
</head>
<body>
{{#if global_config.googleTagManagerId}}
Expand All @@ -153,7 +159,7 @@
height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript>
<!-- End Google Tag Manager (noscript) -->
{{/if}}
<div class="YxtPage-wrapper {{pageWrapperCss}}">
<div {{#if (isRTL global_config.locale) }}dir="rtl"{{/if}} class="YxtPage-wrapper {{pageWrapperCss}}">
{{> overlay/markup/overlay-header }}
{{> layouts/header }}
<div class="YxtPage-content">
Expand Down
12 changes: 12 additions & 0 deletions static/js/rtl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* Returns true if the given language is written from right to left (requires rtl css)
*
* @param {string} locale
* @returns {boolean}
*/
module.exports = function isRTL (locale) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if(locale === 'ar') {
yen-tt marked this conversation as resolved.
Show resolved Hide resolved
return true;
}
return false;
}
89 changes: 89 additions & 0 deletions static/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions static/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"mini-css-extract-plugin": "^1.6.0",
"postcss": "^8.3.1",
"resolve-url-loader": "^3.1.1",
"rtlcss-webpack-plugin": "^4.0.6",
"sass": "^1.34.0",
"sass-loader": "^8.0.2",
"simple-git": "^2.15.0",
Expand Down
4 changes: 4 additions & 0 deletions static/scss/answers/theme.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@
margin-top: calc(var(--yxt-base-spacing) / 2);
margin-bottom: calc(var(--yxt-base-spacing) / 4);
}

span {
display: inline-block;
}
}

&-results {
Expand Down
16 changes: 16 additions & 0 deletions static/webpack-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome!

const isRTL = require('./js/rtl');

module.exports = function () {
const jamboConfig = require('./jambo.json');
Expand Down Expand Up @@ -33,6 +35,19 @@ module.exports = function () {
globalConfig = parse(globalConfigRaw);
}

const cssRtlPlugin = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

@yen-tt yen-tt Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay that's fair!

const localeConfigPath = `./${jamboConfig.dirs.config}/locale_config.json`;
if (fs.existsSync(localeConfigPath)) {
localeConfigRaw = fs.readFileSync(localeConfigPath, 'utf-8');
localeConfig = parse(localeConfigRaw);
const hasRtlLocale = Object.keys(localeConfig.localeConfig).some((locale) => isRTL(locale));
if(hasRtlLocale) {
cssRtlPlugin.push(new RtlCssPlugin({
filename: '[name].rtl.css'
}));
}
}

const { useJWT } = globalConfig;

let jamboInjectedData = process.env.JAMBO_INJECTED_DATA || null;
Expand All @@ -53,6 +68,7 @@ module.exports = function () {
}[chunkName] || '[name].css'
}
}),
...cssRtlPlugin,
...htmlPlugins,
new webpack.DefinePlugin({
'process.env.JAMBO_INJECTED_DATA': JSON.stringify(jamboInjectedData)
Expand Down