Skip to content

fix: rtl language layout - #28861

Merged
davidjoy merged 1 commit into
openedx:masterfrom
open-craft:meysam/fix-rtl-language-layout
Oct 18, 2021
Merged

fix: rtl language layout#28861
davidjoy merged 1 commit into
openedx:masterfrom
open-craft:meysam/fix-rtl-language-layout

Conversation

@meysam81

@meysam81 meysam81 commented Sep 28, 2021

Copy link
Copy Markdown
Contributor

Description

So far, every RTL language style has been overwritten in every theme to cover for the unsupported behavior of RTL languages in the platform. This fix will address that and it'll reduce the overhead to do the same in the themes.

With this fix, we won't need to change any theme because the default will be to support the RTL languages even in the face of the default theme.

Reproducing

You'll need to run the devstack, change the language to some RTL language (e.g. ar), and see the style correctly formatted in the default theme and any customized theme that hasn't patched the fix.

Extra info

This PR is in regard to this ticket.

It also is a replacement for this PR.

reviewer @shimulch

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Sep 28, 2021
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @meysam81! I've created OSPR-6077 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@natabene

Copy link
Copy Markdown
Contributor

@meysam81 Thank you for your contribution!

@meysam81
meysam81 changed the base branch from open-release/koa.master to master September 29, 2021 05:35
@meysam81
meysam81 requested a review from a team September 29, 2021 05:35
@meysam81
meysam81 changed the base branch from master to open-release/koa.master September 29, 2021 05:35
@meysam81
meysam81 changed the base branch from open-release/koa.master to master September 29, 2021 05:39
@meysam81
meysam81 force-pushed the meysam/fix-rtl-language-layout branch from e51b83a to d5574d8 Compare September 29, 2021 05:52

@shimulch shimulch left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@meysam81 LGTM 👍

  • I tested this: on the local devstack. Checked the existence of the issue in the master branch and then ensured this PR fixes it.
  • I read through the code
  • [N/A] I checked for accessibility issues
  • Includes documentation
  • [N/A] I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

@meysam81 can you check why CI is failing?

@meysam81

meysam81 commented Oct 4, 2021

Copy link
Copy Markdown
Contributor Author

@natabene

The CI is failing because of some quality checks when running pylint but I didn't touch any python file so there shouldn't be any concern.

cc @shimulch

@davidjoy

Copy link
Copy Markdown
Contributor

As far as I can tell the XSS linter is failing, which is related to templates and HTML. Is it possible for us to rebase this branch onto master and re-run the checks?

@shimulch
shimulch force-pushed the meysam/fix-rtl-language-layout branch from d5574d8 to d21b795 Compare October 16, 2021 03:59
@shimulch
shimulch force-pushed the meysam/fix-rtl-language-layout branch from d21b795 to 6bf4364 Compare October 16, 2021 04:00
@shimulch

Copy link
Copy Markdown

@davidjoy I've rebased & squashed the changes. Seems like there is no failure anymore.

@davidjoy
davidjoy merged commit 9726d14 into openedx:master Oct 18, 2021
@openedx-webhooks

Copy link
Copy Markdown

@meysam81 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

1 similar comment
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@farhaanbukhsh
farhaanbukhsh deleted the meysam/fix-rtl-language-layout branch October 26, 2021 13:21
@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants