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

RTL support for TheAdmin theme #3457

Merged
merged 14 commits into from May 1, 2019

Conversation

Projects
None yet
4 participants
@hishamco
Copy link
Contributor

commented Apr 11, 2019

Fixes #3015

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@agriffard looking for elegant way to run rtlcss instead of gulp rtl

@agriffard

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

I tried the PR:
Launch and setup a Blog site.
Enable the Localization module.
Add the ar culture.
Select it as the default culture.

=> In the admin layout, the @Orchard.CultureName() stays at fr which is my System culture.

Do you remember when you forked OrchardCore on your repo?

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

It's a bit old, but I remember that I rebase to one of the earlier commits .. let me check again

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

As I can see the rtl branch is 3 commits ahead of OrchardCMS:dev.

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

=> In the admin layout, the @Orchard.CultureName() stays at fr which is my System culture.

@agriffard could you try to restart the application, so the added culture will take the affect

@agriffard

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Even if I restart the application, it is not working.

I can only see it if I suffix the url by something like this: ?culture=ar-YE

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Strange!! just review the RTL layout and please answer my question in the first thread, then we will check why the localization isn't working properly

@agriffard

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

After adding the arabic .po files, it works as expected:
image

Login Page:
image

I don't know how you can use something else than gulp rtlcss to build the -rtl css file.

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Strange!! it worked at my end without adding Arabic.po file this is just for localise the resources nothing else .. it's great to see RTL shine in Orchard Core, but we need to investigate on the localization issue that you come up with

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@khoshroomahdi could you please try out Persian (Farsi) at your end and check how the RTL layout is look like, we need more than one to convince the others that Orchard Core is RTL ready 😁

@@ -1,4 +1,4 @@
<div class="alert alert-success">
<h4>@T["Welcome to Orchard"]</h4>
<p>@T["Feel free to browse the menu on the left and discover all its possibilities."]</p>
<p>@T["Feel free to browse the menu and discover all its possibilities."]</p>

This comment has been minimized.

Copy link
@sebastienros

sebastienros Apr 16, 2019

Member

Maybe it should just be a different translation for rtl languages ;)

This comment has been minimized.

Copy link
@hishamco

hishamco Apr 16, 2019

Author Contributor

May be, but I remove it to avoid the confusing

gulpfile.js Outdated
@@ -30,6 +31,14 @@ require("events").EventEmitter.prototype._maxListeners = 100;
** GULP TASKS
*/

gulp.task("rtl", function() {
var styleFolder = "./src/OrchardCore.Themes/TheAdmin/wwwroot/Styles";

This comment has been minimized.

Copy link
@sebastienros

sebastienros Apr 16, 2019

Member

No. We can't have a specific reference to this css in this file.
Also why only this file and not all the other ones?

This comment has been minimized.

Copy link
@hishamco

hishamco Apr 16, 2019

Author Contributor

That's why I asked above, I'm not gulp expert, so we need a better way for doing this

@sebastienros

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Why not check what the RTLCSS did, and apply all the changes on the rtl selector that is defined in <body> only once, and then when we do a change in TheAdmin we need to ensure it also works for RTL?

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Why not check what the RTLCSS did, and apply all the changes on the rtl selector that is defined in only once, and then when we do a change in TheAdmin we need to ensure it also works for RTL?

I already did that few days ago, it's not easy as you adding rtl selector to the body, it brings a new style sheet that you need to inject

What I did is the simple way to support the RTL, except we need an elegant way to run the gulp task

<div class="alert alert-success">
<h4>@T["Welcome to Orchard"]</h4>
<p>@T["Feel free to browse the menu and discover all its possibilities."]</p>
<p>@T[$"Feel free to browse the menu on the {(OrchardHelper.CultureDir() == "ltr" ? "left" : "right")} and discover all its possibilities."]</p>

This comment has been minimized.

Copy link
@sebastienros

sebastienros Apr 16, 2019

Member

It's worse than anything. A translated string can't be dynamic, we can't find the values automatically. And even if in this case there are only two possibilities, it's bad. Either remove the term as you did earlier, or leave it as it was initially and we can handle it in a localization entry.

This comment has been minimized.

Copy link
@hishamco

hishamco Apr 16, 2019

Author Contributor

So that's why I remove it at the beginning :)

hishamco added some commits Apr 16, 2019

Use RTL inside build CSS pipeline
Remove rtl gulp task
Use rtl inside css pipeline

@hishamco hishamco changed the title [WIP] RTL support for TheAdmin theme RTL support for TheAdmin theme Apr 25, 2019

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@sebastienros as we discusses yesterday on Slack, I spent few hours to get this done with a single style file with no luck :( but I'm so close .. the problem is i'm not a gulp expert 😄

I think your suggestion is very good for now.

  • I added generateRTL to Assets.json to avoid hard coded names in gulp file
  • I modified the LinkTagHelper, so support RTL will be super easy

I will push my changes ASAP

Few enhancements
- Use generateRTL option in `Assert.json` to avoid hard coded style filenames
- Improve the RTL gulp function
- RTL style will be generated from within taghelper
@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

The only thing that I need to find somehow is why the rtl is not generate the RTL files when both TheAdmin.css and TheAdmin.min.css are exist

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

It works fine when I run gulp rebuild if that is the right behavior the PR is ready from my side

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Finally I did it 😄 .. I spent several hours to make both language direction into a single style, believe it or not most of the time wasted on css-prefix which is limited and another package that I am using and finally discovered there was an error in the docs 😣
What I did is the following:

  • Remove the earlier changes in the LinkTagHelper it is not needed anymore
  • Modify the gulp file so the RTL will run during the css pipeline generation
  • Modify the CultureDir() so it should return ltr instead of empty string

Hopefully @sebastienros will be supper happy because he insists likes that both language direction styles should go into a single file 😀

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Seems I made a change accidently on the login page when I was working on PR #3518 it moved to here while login is related to admin

"typescript": "3.2.2"
},
"dependencies": {
"gulp-rtlcss": "^1.3.0",

This comment has been minimized.

Copy link
@sebastienros

sebastienros May 1, 2019

Member

I don't think you need this dependency anymore.

@@ -1,4 +1,5 @@
using System;
using System;
using System.Globalization;

This comment has been minimized.

Copy link
@sebastienros

sebastienros May 1, 2019

Member

Change not necessary

@sebastienros

This comment has been minimized.

Copy link
Member

commented May 1, 2019

There are also merge conflict, so you need to rebase your branch (or merge dev), ignore any change in TehAdmin (for simplicity of merging) then regenerate the css files (gulp rebuild) and commit again. Then I'll merge

sebastienros added some commits May 1, 2019

Merge remote-tracking branch 'origin/dev' into rtl
# Conflicts:
#	src/OrchardCore.Themes/TheAdmin/wwwroot/Styles/TheAdmin.css
#	src/OrchardCore.Themes/TheAdmin/wwwroot/Styles/TheAdmin.min.css
@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Sure

sebastienros added some commits May 1, 2019

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Seems you did most what I did locally .. but I'm facing Internet issues .. thanks

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

I think it's time to merge

@sebastienros

This comment has been minimized.

Copy link
Member

commented May 1, 2019

it is, but I am doing some tests, currently facing issues with crowdin configuration, on it

@sebastienros sebastienros merged commit 58096e8 into OrchardCMS:dev May 1, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@hishamco hishamco deleted the hishamco:rtl branch May 2, 2019

@aghili371

This comment has been minimized.

Copy link

commented May 2, 2019

it has a problem in media popup and assets, the still left to right and the tree will hide.

2
Uploading 1.JPG…

@hishamco

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

@aghili371 could you please file an issue so it can easily tracked, I will try to fix it as soon as possible.

I'm not sure if it's inline or internal style because it will not work with the current implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.