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

Support RTL languages - migrate css from physical to logical positioning #1654

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

susnux
Copy link
Collaborator

@susnux susnux commented Jun 25, 2023

Summary

This should at least resolve the RTL issue on our side, but it still looks odd when having a body set to dir=rtl because the @nextcloud/vue package needs to be updated as well.

Screenshots

I checked LTR before / after => exactly the same.

For RTL languages it looks like the following, note that elements (and e.g. the "answers are connected to your account" text) are still left aligned. This should be fixed by changing the @nextcloud/vue package and global nextcloud theme.
So currently only elements / texts changed by the user are handled by setting the direction to auto, because even though the language is a RTL one (this example uses Persian) users could use English text (name, etc).

before after
image image

For the question note that the first one is "Persian English" and the second one is "English Persian". And note that the * is on the after screenshot aligned on the left as it should be on the end of the question.

Notes

@ahangarha what do you think? Does this work for you?

(Generally I would suggest to next fix the Nextcloud vue package, but this requires much more work.)

@susnux susnux added bug Something isn't working design Related to the design 3. to review Waiting for reviews labels Jun 25, 2023
@susnux susnux changed the title Fix/alignments rtl Support RTL languages - migrate css from physical to logical positioning Jun 25, 2023
@Chartman123
Copy link
Collaborator

@susnux could you perhaps also add some screenshots to the first post with before/after for a RTL language? Or a LTR/RTL comparison? :)

@susnux
Copy link
Collaborator Author

susnux commented Jun 25, 2023

@Chartman123 done ✔️


ℹ️ This is a mockup:

A full RTL version would look like this (I guess), but as said it requires the vue library to be fixed and a global RTL setting:
image

Copy link

@ahangarha ahangarha left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

It looks good.

The only thing that requires a double check is lists. Fill content and ensure they behave correctly.

Generally they are two rules:

  • li shouldn't get dir="auto"
  • In nested elements, the first child shouldn't get dir="auto".

You have applied logical style properties very well. 👍🏾

If there is any test server, please let me know so I can check to ensure everything works fine both while creating the form and when it is being filled.

@Chartman123
Copy link
Collaborator

@ahangarha you can just spin up a local Nextcloud instance and follow the instructions on the Readme file of this repository to get your local clone of this repo running. :)

@ahangarha
Copy link

@Chartman123 I try to do it this week and give feedback.

@jancborchardt
Copy link
Member

Awesome! Also cc @mabkenar who had feedback about this some time ago – if you want to check it out. :)

@Chartman123 Chartman123 added this to the 3.4 milestone Jun 26, 2023
@ahangarha
Copy link

I tested the branch on a local setup.
Some features work as expected but in many cases we still need tweaks.

I think the best way to move forward from here is that I make a PR to fix/alignments-rtl and continue its work. In some cases (parsing markdown to html, we may use https://github.com/dobidi/markdown-it-bidi plugin developed by me and and one of my good friends. We need to apply some new changes)

How is it?

@susnux
Copy link
Collaborator Author

susnux commented Jul 5, 2023

@ahangarha what kind of issues do you face?

Are the issues related to the alignment within the forms app?
Because I think it is often easier to have pull requests small, so e.g. first merge the PR with the change from physical to logical alignments (padding-left vs padding-inline-start) like the first commit of this one.

And then a second pull request fixing the dir like the mentioned markdown part.

It is just that huge PRs with all in one are hard to review, so doing it step by step (if possible without breaking anything) would be easier.

But help is highly welcomed 😃
I think there is still much work, especially as we use components from https://github.com/nextcloud/nextcloud-vue which do not support RTL.

@ahangarha
Copy link

@susnux You are right. I didn't see any problematic issue. I thought better to make improvements but I agree with you. One small step at a time is better.

I will make another PR making further improvement. This is perhaps a better approach.

src/components/Results/Submission.vue Outdated Show resolved Hide resolved
src/components/Results/ResultsSummary.vue Outdated Show resolved Hide resolved
src/components/Questions/Question.vue Outdated Show resolved Hide resolved
src/views/Results.vue Outdated Show resolved Hide resolved
…*-block`, `*-inline`)

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
… form description)

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
Copy link
Collaborator Author

susnux commented Jul 5, 2023

@Chartman123 Resolved your comments and rebased onto current main branch 😃

@ahangarha Sure create a pull request, any help is welcome 🙏

@susnux susnux requested a review from Chartman123 July 5, 2023 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working design Related to the design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add RTL and preferably, bidi support
4 participants