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

Use logical positioning instead of physical for bidirectional text support #47343

Merged
merged 7 commits into from
Aug 29, 2024

Conversation

AndyScherzinger
Copy link
Member

@AndyScherzinger AndyScherzinger commented Aug 20, 2024


Summary

This PR is the first step towards adding bidirectional text support to the Nextcloud ecosystem. It affects the entire project; accordingly, each app should have subsequent PRs.

The PR is 90+ percent complete. I decided to make the PR to

  • Get early feedback to ensure it is on the right path.
  • Discuss some technical challenges.

I had challenges in building the project. Generating css files and the dist directory during development is something I am uncomfortable with.

Because of supporting Samsung Internet, I needed to take some hacky approach.

Note:
We must update many components in NextCloud/vue. The most important one was the header-menu vue component which is already updated. Others will come soon. First, I need to ensure this PR is getting prepared for merge.

Screenshots

Before After
image image
image image

TODO

Checklist

@skjnldsv
Copy link
Member

skjnldsv commented Aug 20, 2024

Files potentially missing some code change from previous PR:

  • apps/files_reminders/src/components/setcustomremindermodal.vue
  • apps/files_sharing/src/components/sharingentryquickshareselect.vue
  • apps/settings/src/components/applist/appitem.vue
  • apps/settings/src/components/personalinfo/emailsection/email.vue
  • apps/settings/src/views/apps.vue
  • apps/updatenotification/src/components/updatenotification.vue
  • apps/user_status/src/userstatus.vue
  • core/src/components/usermenu/usermenuentry.vue
  • core/src/views/usermenu.vue

@nickvergessen
Copy link
Member

Backend part moved to #47349

core/src/views/Profile.vue Outdated Show resolved Hide resolved
@susnux susnux force-pushed the feat/31420/add-bidi-support branch from 6e73d75 to 55dd692 Compare August 20, 2024 17:42
@susnux susnux changed the title WIP - Add bidirectional text support Use logical positioning instead of physical for bidirectional text support Aug 20, 2024
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Test seems to work fine, but some comments.

apps/dashboard/src/DashboardApp.vue Outdated Show resolved Hide resolved
apps/dashboard/src/DashboardApp.vue Outdated Show resolved Hide resolved
apps/files/css/detailsView.scss Outdated Show resolved Hide resolved
apps/files/css/files.scss Outdated Show resolved Hide resolved
apps/files/css/files.scss Outdated Show resolved Hide resolved
core/css/guest.scss Outdated Show resolved Hide resolved
core/css/guest.scss Outdated Show resolved Hide resolved
core/css/systemtags.scss Outdated Show resolved Hide resolved
core/src/views/LegacyUnifiedSearch.vue Outdated Show resolved Hide resolved
core/src/views/LegacyUnifiedSearch.vue Outdated Show resolved Hide resolved
@susnux susnux force-pushed the feat/31420/add-bidi-support branch from 4527b31 to b789d58 Compare August 20, 2024 21:04
@susnux susnux force-pushed the feat/31420/add-bidi-support branch 3 times, most recently from 7ccea6d to 7e6f6fd Compare August 27, 2024 20:10
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Tested it, could not find any regressions with LTR language.
So I think we should be able to merge even before the backend PR which allows RTL layout.

@susnux susnux force-pushed the feat/31420/add-bidi-support branch from 7e6f6fd to 3543ae0 Compare August 27, 2024 20:20
@skjnldsv
Copy link
Member

Ready for review @susnux ?

@susnux
Copy link
Contributor

susnux commented Aug 28, 2024

Ready for review ?

Yes I think so

@susnux susnux marked this pull request as ready for review August 28, 2024 12:20
@susnux susnux requested review from artonge and ShGKme August 28, 2024 12:21
@nickvergessen nickvergessen removed their request for review August 28, 2024 12:51
@AndyScherzinger AndyScherzinger added this to the Nextcloud 31 milestone Aug 28, 2024
@skjnldsv
Copy link
Member

/compile amend-rebase /

ahangarha and others added 7 commits August 29, 2024 08:32
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
Fix other background-positions
Minor fix in link button icon position
Update header left and right to start and end

Signed-off-by: Mostafa Ahangarha <ahangarha@riseup.net>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Aug 29, 2024
@skjnldsv skjnldsv merged commit 292a306 into master Aug 29, 2024
173 of 175 checks passed
@skjnldsv skjnldsv deleted the feat/31420/add-bidi-support branch August 29, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ☑️ Done
Development

Successfully merging this pull request may close these issues.

5 participants