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

[TASK-548] NLP add rtl script support #4904

Merged
merged 3 commits into from Apr 15, 2024
Merged

[TASK-548] NLP add rtl script support #4904

merged 3 commits into from Apr 15, 2024

Conversation

magicznyleszek
Copy link
Member

Checklist

  1. If you've added code that should be tested, add tests
  2. If you've changed APIs, update (or create!) the documentation
  3. Ensure the tests pass
  4. Make sure that your code lints and that you've followed our coding style
  5. Write a title and, if necessary, a description of your work suitable for publishing in our release notes
  6. Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
  7. Open an issue in the docs if there are UI/UX changes

Description

Any transcript or translation text written in RTL script will be displayed correctly.

Notes

The dir='auto' attribute is magically doing all the job. I thought there would be a way to set it as default for every HTML node, but it doesn't seem to work. Adding it to each node with RTL text does the job.

@magicznyleszek magicznyleszek marked this pull request as ready for review April 15, 2024 08:37
@magicznyleszek magicznyleszek changed the title NLP add rtl script support [TASK-548] NLP add rtl script support Apr 15, 2024
Copy link
Contributor

@LMNTL LMNTL left a comment

Choose a reason for hiding this comment

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

Works for me! I have a functional question about the Django templates, but it's not a blocker - the addition seems benign, just not sure what it's doing.

@@ -2,7 +2,7 @@

{% load static %}
<!doctype html>
<html>
<html dir="auto">
Copy link
Contributor

@LMNTL LMNTL Apr 15, 2024

Choose a reason for hiding this comment

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

Does this line do anything? As you noted in the PR description, dir="auto" doesn't appear to transfer down to child elements. I can't find a difference on /accounts/login/ - the layout, text direction, and input text direction appear to all stay the same as on beta, whether using an RTL or LTR language.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that although it doesn't transfer down, it does something sometimes :P I tried to find any documentation or SO question that would confirm that, but there is nothing. I removed it

@magicznyleszek magicznyleszek merged commit fa92b5f into beta Apr 15, 2024
3 of 4 checks passed
@magicznyleszek magicznyleszek deleted the nlp-rtl-support branch April 15, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants