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

allow right to left styling of docs #34486

Merged
merged 1 commit into from
Nov 19, 2018
Merged

Conversation

tamarr
Copy link
Contributor

@tamarr tamarr commented Nov 19, 2018

Summary

Adding ability to specify rtl for guides that will generate right to left guides with appropriate styling.

Other Information

Guides will still generate left-to-right by default. A new environment variable will be checked 'RTL=true' which will generate the guides with right to left styling.

The new stylesheet main.rtl.css is copied from main.css with right and left swapped as well as some other updates to padding and margin. When the switch is passed - main.css will be deleted and main.rtl.css with be renamed to main.css. While changes to main.css will have to be also ported to main.rtl.css, manipulating the file itself from the generator did not seem right.

The anchor comparison was updated to unescaped since html escapes the Hebrew chars and the comparison fails.

Here are some basic examples, not fully translated:

screen shot 2018-11-18 at 2 46 48 pm

screen shot 2018-11-18 at 2 50 33 pm

* adding rtl css main file and logic to use
@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @georgeclaghorn (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@@ -116,6 +117,10 @@ def select_only(guides)

def copy_assets
FileUtils.cp_r(Dir.glob("#{@guides_dir}/assets/*"), @output_dir)
if (@rtl)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you don't need parentheses.

@@ -0,0 +1,762 @@
/* Guides.rubyonrails.org */
/* Main.css */
Copy link
Contributor

Choose a reason for hiding this comment

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

Although it would be copied to main.css, it may be useful to maintain here that it is in fact Main.rtl.css.

@@ -17,13 +17,14 @@ module RailsGuides
class Generator
GUIDES_RE = /\.(?:erb|md)\z/

def initialize(edge:, version:, all:, only:, kindle:, language:)
def initialize(edge:, version:, all:, only:, kindle:, language:, rtl: false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding variable naming, I would prefer to have right_to_left instead of rtl for better readability.

@rafaelfranca rafaelfranca merged commit 29e0ed1 into rails:master Nov 19, 2018
rafaelfranca added a commit that referenced this pull request Nov 19, 2018
allow right to left styling of docs
@albertoalmagro
Copy link
Contributor

@rafaelfranca regarding my naming suggestion, without the context of this PR I wouldn't know what RTL means, the first thing that comes to my mind when I read RTL is the german TV channel. Other parameters in the constructor are algo in this fashion (ex. language instead of lang or something else). Would you mind if I open a PR to change the naming ro right_to_left instead of rtl?

@paracycle
Copy link
Contributor

@albertoalmagro I would suggest that we rename the parameter to direction instead, with accepted values :ltr and :rtl (with the default being :ltr). That would be inline with how CSS handles the same use-case and would be more understandable at the callsite of the method, i.e. direction: :rtl as opposed to right_to_left: true.

What do you think @rafaelfranca?

@rafaelfranca
Copy link
Member

Agree with @paracycle.

@tamarr
Copy link
Contributor Author

tamarr commented Nov 22, 2018

Sounds good, I'll create the pr on Sunday. I'll also add this info to the docs on translating the docs :)

@albertoalmagro
Copy link
Contributor

Thanks @tamarr but there is not need, at least for the direction change, if you read again my comment I had already volunteered to submit a PR about that. Regarding docs translation... all yours!

On the other hand, I would like to give @paracycle credit as I think his suggestion is key 👍

@tamarr
Copy link
Contributor Author

tamarr commented Nov 29, 2018

thanks @albertoalmagro for the comments and the update and sorry for the delayed response, having crazy weeks at work 😅

and thanks @paracycle for the review and suggestions!

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

Successfully merging this pull request may close these issues.

None yet

6 participants