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

Bidi support #2357

Merged
merged 20 commits into from Sep 15, 2017

Conversation

@samarsultan
Copy link
Contributor

samarsultan commented Apr 2, 2017

Hello @Carreau ,

This pull request contains the bidi work and fixes the UI mirroring issues.

Here are some functions added such : LoadLocale -> to load the required locale that is defined at Momentjs based on the browser preferences setting . I propose to leave it to the user selection rather than the browser preferences and to create a UI component to toggle among the list of locales.

For Numeric Shaping : I need another component to toggle between the options.

With respect to National Calendar , I found an external plugin at Momentjs called moment-hijri that is dedicated to Hijri-calendar so I removed the related functions that were mentioned at the design. Actually I didn't find moment at the documentation . I'll put the links below :

Could you please review ?
Waiting your comments. Thanks !

@samarsultan samarsultan changed the title Bidi support Bidi support #2178 Apr 2, 2017

@samarsultan samarsultan changed the title Bidi support #2178 Bidi support Apr 2, 2017

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Apr 3, 2017

This approach looks great! Having a user toggle for locale makes sense to me.

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Apr 3, 2017

pinging @ellisonbg for design consideration.

@samarsultan

This comment has been minimized.

Copy link
Contributor Author

samarsultan commented Apr 9, 2017

Could you please review ?

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Jul 27, 2017

@samarsultan Can you please resolve the merge conflicts for this PR?

What is the current status? We are closing in on 5.1 release and if this is complete, then let's try to get it in there!

@samarsultan

This comment has been minimized.

Copy link
Contributor Author

samarsultan commented Aug 1, 2017

@gnestor ,Conflicts have been resolved.

The current status is having a mirrored UI with loading the dedicated Locale form momentJS.
For Numeric Shaping , I left it to the browser language ,instead of user selection, while loading the locale. So if the language is "ar" , you will see the Arabic locale is loaded and numbers are displayed in Arabic-Indic format.

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Aug 1, 2017

@samarsultan Excellent!! How do I set locale or enable mirroring?

@samarsultan

This comment has been minimized.

Copy link
Contributor Author

samarsultan commented Aug 1, 2017

For now, you can do it by just setting the browser language to "ar" . However, it will be better to have a user toggle for locale as proposed above.

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Aug 1, 2017

Doesn't seem to be working for me. Here's what my Chrome settings look like:

image

The notebook and dashboard are rendering LTR as usual...

@samarsultan

This comment has been minimized.

Copy link
Contributor Author

samarsultan commented Aug 1, 2017

You have also to ensure that "Display Google Chrome in this Language" is checked and probably you need to relaunch your browser.

chrome language setting

Or you can simply do it from Firefox by just moving up the Arabic language. It will be easier to return it back to English.

firefox language

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Aug 2, 2017

Ok, it's working in Firefox. It looks great to me!

It looks like we can include this in 5.1 release assuming that there are no objections. @Carreau @minrk Would you care to review?

@gnestor

gnestor approved these changes Aug 2, 2017

@gnestor gnestor requested review from minrk and Carreau Aug 2, 2017

@gnestor gnestor added this to the 5.1 milestone Aug 2, 2017

@gnestor gnestor referenced this pull request Aug 2, 2017

Closed

Release 5.1 #2708

11 of 11 tasks complete
@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Aug 3, 2017

I'm going to mark this as 5.2 and will merge into master as soon as 5.1 is released. This will everyone a chance to use it before it's released.

@gnestor gnestor modified the milestones: 5.2, 5.1 Aug 3, 2017

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Sep 15, 2017

5.1 is released so let's merge this and start testing it!

@gnestor gnestor merged commit aa3c1a5 into jupyter:master Sep 15, 2017

4 checks passed

codecov/patch 30% of diff hit (target 0%)
Details
codecov/project 79.73% (-0.03%) compared to dbda330
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
if(_isMirroringEnabled()){
$("body").attr("dir","rtl");
}
requirejs(['components/moment/locale/'+_uiLang()], function (err){

This comment has been minimized.

@rgbkrk

rgbkrk Sep 25, 2017

Member

I think we need to special case en and en-us since the files are not there and are the builtins from moment: moment/moment#2081

This comment has been minimized.

@gnestor

gnestor Oct 9, 2017

Contributor

Looks like you already took care of it! 👍

This comment has been minimized.

@rgbkrk

rgbkrk Oct 9, 2017

Member

Oh yeah I should have noted that again here. Good to go now.

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Oct 9, 2017

Notebook 5.2.0rc1 is available on PyPI so please give it a try 👍:

pip install notebook --pre --upgrade

@samarsultan

This comment has been minimized.

Copy link
Contributor Author

samarsultan commented Nov 27, 2017

Hello @gnestor and @rgbkrk.

Thanks for reviewing our code.
Could you please update the "README" file and the documentation with the way of enabling the above features?

If you have any comments, @ragabo will take over.

@gnestor

This comment has been minimized.

Copy link
Contributor

gnestor commented Dec 1, 2017

@takluyver Should we add the following from https://github.com/jupyter/notebook/blob/master/notebook/i18n/README.md#how-the-language-is-selected- to the README?

jupyter notebook command reads the LANG environment variable at startup, (xx_XX or just xx form, where xx is the language code you're wanting to run in).
Hint: if running Windows, you can set it in PowerShell with ${Env:LANG} = "xx_XX".

The preferred language for web pages in your browser settings (xx) is also used. At the moment, it has to be first in the list.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Dec 1, 2017

I'd focus on getting the machinery for building and contributing translations in place first.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Dec 1, 2017

I've opened a couple of issues for that - #3102, #3103

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.