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

Adding GUI Mirroring- issue #1852 #1860

Merged
merged 5 commits into from
Oct 31, 2016
Merged

Adding GUI Mirroring- issue #1852 #1860

merged 5 commits into from
Oct 31, 2016

Conversation

samarsultan
Copy link
Contributor

Hey @rgbkrk .
Please check this PR, it is a sample of applying the proposed design approach of UI mirroring.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 30, 2016

Cool, how do I enable this? Do I switch my language to Arabic on my OS?

@samarsultan
Copy link
Contributor Author

No, just from the browser settings.

@Carreau
Copy link
Member

Carreau commented Oct 31, 2016

Great ! Thanks a lot @samarsultan to work on that !

I guess for test purpose, we can add a notebook command to force-switch rtl.
Also I'm pretty sure you tried, but are all the !important necessary ?

I'm happy to merge that early and iterate.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 31, 2016

I'm cool bringing it in and iterating as well, we look forward to you iterating on it @samarsultan. 😄

@rgbkrk rgbkrk merged commit 09b2a8b into jupyter:4.x Oct 31, 2016
@samarsultan
Copy link
Contributor Author

@Carreau ,thanks for your add-on . You are right , we may not need all the important except for 1 or 2 places. I'll re-push them .

@samarsultan
Copy link
Contributor Author

@rgbkrk , do you mean by iterating on it to be fully supported ? If so , I just needed to gurantee that our approach is going to be suitable to submit the full patch.

@Carreau
Copy link
Member

Carreau commented Nov 2, 2016

I think that by "iterating" we mean:

Feel free to submit partial patches, small patches are easier to review quickly and merge.

The time to review and land some changes increase with the size of the patch.

It's even ok to get something in master have it available for a week or so and decide that it was not actually the right thing to do.

Does that make some sens ?

Thanks !

@rgbkrk
Copy link
Member

rgbkrk commented Nov 2, 2016

By iterating, I mean that we should merge this and not expect perfection right away. We should make forward progress with you and continue patching to get closer to the ideal. I'm not a domain expert on this, so I want you to have plenty of freedom to support rtl for users. 😄

@minrk minrk added this to the 5.0 milestone Nov 23, 2016
@samarsultan
Copy link
Contributor Author

With regard to this PR , I'm going to continue supporting the UI Mirroring alongside with the new requested features #2154 & #2156 ,if you are OK with them.

I need to ensure which branch to work against. Do I need to create a new branch to push my changes in (for ex. "bidi" branch)?

Thanks !

@Carreau
Copy link
Member

Carreau commented Feb 9, 2017

With regard to this PR , I'm going to continue supporting the UI Mirroring alongside with the new requested features #2154 & #2156 ,if you are OK with them.

Of course, we welcome your input and contribution on this !

I need to ensure which branch to work against. Do I need to create a new branch to push my changes in (for ex. "bidi" branch)?

You likely want to always submit your work against master (that's the default GitHub configuration so that should be easy), but the exact branch you push your work to, does not really matter. You should anyway be able to push only on your fork. The only limiting factor is you need at least one branch per opened Pull Request. So if you are sure to always have only one PR "in flight" then you can always use the same "bidi" branch. Otherwise you can push things with different name like bidi-fix-calendar, and bidi-fix-numeral for example, that what I would do.

Also, just as a tip, we tend to not check messages on close issues/merged PR, so if we don't respond feel free to open a new issue. If you have any other questions about git feel free to ask.

@samarsultan
Copy link
Contributor Author

Thanks @Carreau , I 'll open a new Issue with the proposed design for the Bidi work.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants