Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

RTL major refactor #5055

Merged
merged 17 commits into from
Oct 30, 2018
Merged

RTL major refactor #5055

merged 17 commits into from
Oct 30, 2018

Conversation

yehudab
Copy link
Contributor

@yehudab yehudab commented Oct 19, 2018

Fixes: #4261
Changes include:

  1. Use CSS Logical Properties and Values in the SCSS sources
  2. Add dir="rtl" or dir="ltr" to <html> tag according to the browser language
  3. Use PostCSS tools to generate separate CSS files for .rtl and .ltr modes, and serve the correct file according to the page direction. This was done as a (hopefully) temp measure until CSS Logical becomes more supported in all major browsers.
  4. Other small fixes, like correct position of the delete-shot-button dialog, or correct position for the screenshots pagination.

ianb
ianb previously requested changes Oct 24, 2018
Copy link
Contributor

@ianb ianb left a comment

Choose a reason for hiding this comment

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

Phew, I was a little intimidated by all the changes, but once I got into the flow all the CSS changes are the same pattern.

One little issue about the isRtl function, otherwise it's looking really good. Thanks for all this work!

server/src/l10n.js Outdated Show resolved Hide resolved
@@ -70,7 +70,7 @@
@keyframes slide-left {
0% {
opacity: 0;
transform: translate3d(160px, 0, 0);
transform: translate3d(160px, 0, 0); // FIXME: RTL
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend to leave the FIXME here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually yes.
My experience with CSS animation is not that good, so I was hoping to either learn some more in order to fix it better, or get help form the community.
In any case, I don't think it should stop the merge of this PR, since it only affects the on-boarding flow.

server/src/pages/shotindex/view.js Outdated Show resolved Hide resolved
@ianb ianb dismissed their stale review October 30, 2018 17:59

resolved

@ianb
Copy link
Contributor

ianb commented Oct 30, 2018

I'm not sure I resolved those package-lock conflicts correctly, but who can ever know? I'll try to regenerate them after this lands.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RTL] Add RTL support for screenshots.firefox.com
2 participants