RTL direction support via properties panel (revised) #1213

okaeiz · 2024-06-18T04:20:49Z

Has worked on #1205

This PR (within the the first commit) adds a new form-js entry named DirectionEntry to the properties panel AppearanceGroup that is activated for the form component (default type).

It is also intended to modify form components to adhere to the direction setting that is selected from the DirectionEntry.

okaeiz · 2024-06-18T07:37:19Z

Here's the demo for Action and Selection components:

RTL-Demo-Selection.Action.mp4

…omponents

okaeiz · 2024-06-18T09:43:14Z

Here's the demo for Input, Presentation and Containers components:

RTL-Demo-InputContainersPresentation.mp4

okaeiz · 2024-06-18T09:52:22Z

This looks completed to me. Here's a recap:

I created a new PR with 3 commits and less diff:
- Properties panel and DirectionEntry
- Input, Presentation, and Containers components
- Action and Selection components

I did NOT change the HTML View because its direction can already be changed via CSS attributes.
I did NOT change iframe, separator, and spacer.
I did NOT change expression because it's always LTR.
I did NOT include any merge commits as you requested.

@Skaiir I'd appreciate it if you take a look at this.

Skaiir · 2024-06-19T08:14:09Z

Currently working on a review of this @okaeiz. Amazing work already, although there will need to be a few things changed before we can approve :D. But I'm confident together we'll get this into the library rather soon ;)

Skaiir

I've mentioned a few things, but overall the direction is very good. There are a few things missing, although we don't need to tackle everything (but would be nice):

Unimplemented
- HTML should also be covered as a default (if it is relatively simple), yes you can define it within the code, but having a single configuration handle everything is just generally good.
- The field placeholders (1*) should probably be RTL as well, just for visual consistency. But this isn't super important.
- Iframe title should also be aligned (iframe content shouldn't be modified though, that is correct)
Issues
- Delete icon overlapping label (2*), would flip the button alignment
- Separator for the number field on the wrong side (2*) (should be flipped)
- Spacing between button and text in dynamic lists are squashed (4*), they might need some cleaning up

1*

2*

3*

4*

For these smaller things, what will be needed is to make adjustments to the styles. So what I would do is, based on the direction setting, set a new class to the root of the form renderer. Then we'd modify the CSS using this tag to adjust those small things.

As mentioned, regarding the fonts, we can discuss this at the end.

Also, ensure that you pass our CI via the npm run all command. It'll show you any issues with the code.

It's a lot of text on my part, but it shouldn't be a ton of changes. You don't need to close the PR, just force push your fixes to the PR.

Nice work so far, this will be a very useful change.

Skaiir · 2024-06-19T08:23:23Z

packages/form-js-editor/src/features/properties-panel/entries/DirectionEntry.js

+import { get } from 'min-dash';
+import { useService } from '../hooks';
+import { SelectEntry, isSelectEntryEdited } from '@bpmn-io/properties-panel';
+import { useDirection } from '../../../../../form-js-viewer/src/render/context/DirectionContext';


The issue here is that you're directly referencing the file across packages. See DateTimeConstraintsEntry for an example of how you can reference the viewer library (you have to ensure it is exported from the viewer).

What I would do is, in the properties-panel hooks file, add an export referencing the viewer package, and then in this file reference the hook with useService

import { useService, useDirection } from '../hooks';

Just to keep things clean.

packages/form-js-editor/src/features/properties-panel/entries/DirectionEntry.js

Skaiir · 2024-06-19T08:28:58Z

packages/form-js-editor/src/features/properties-panel/entries/index.js

@@ -39,3 +39,5 @@ export { RowCountEntry } from './RowCountEntry';
 export { HeadersSourceSelectEntry } from './HeadersSourceSelectEntry';
 export { ColumnsExpressionEntry } from './ColumnsExpressionEntry';
 export { StaticColumnsSourceEntry } from './StaticColumnsSourceEntry';
+export { DirectionEntry } from './DirectionEntry';


If you're using vscode, install the prettier code formatter app, and also eslint, it will show you errors you're missing and adhere to our formatting rules.

If you're using another software to code I'm not sure if they have support for it but either way both have CLIs, so you can use the CLI to fix things up.

packages/form-js-editor/src/features/properties-panel/groups/AppearanceGroup.js

Skaiir · 2024-06-19T08:34:14Z

packages/form-js-viewer/src/render/components/form-fields/Button.js

+  const form = useService('form');
+  const { schema } = form._getState();
+
+  const direction = schema?.direction || 'ltr'; // Fetch the direction value from the form schema


I think, you should try and go for a single useDirection hook like you had in the properties panel. The reason for this is that, firstly it's more readable, but also if we decide to change how it behaves, we only need to change it in on spot. It's also less error prone.

Skaiir · 2024-06-19T09:03:00Z

packages/form-js-viewer/src/render/components/form-fields/Button.js

+      class={formFieldClasses(type)}
+      style={{
+        direction: direction,
+        fontFamily: 'Vazirmatn, sans-serif',


We have a css variable that defines fontFamily, so you can set it across the entire application rather easily (in general you'll find we don't use inline styles much, except for alignment). So if we're implementing it within the form-js, we should do it in one place only (somewhere in the form root).

For now, because I still need to understand a little bit more about the font stuff, please just remove it. We'll get back to it when everything else is done.

PS: the comments on this file apply across the board.

I removed the font; but I kindly ask you to reconsider this decision. The font makes no difference to English and the languages that have a similar alphabet to English. But in Persian, it is one of the main (if not the main) reasons why people decide not to use a software.

If you take a look at Github, you'll find out that Github is also using Vazirmatn for its Persian strings. It is highly admired among open-source community.

okaeiz · 2024-06-26T10:51:00Z

I made a new commit. Here's a recap:

Implemented
- I covered HTML View to adhere to direction.
- I covered iFrame to adhere to direction.
Issues fixed
- Icon overlapping for DynamicList is fixed both in Editor and Viewer
- Separator for the number field is now on the right side
- Removed unnecessary comments
- Removed the font attribute (temporarily) ---> will be discussed

I did NOT change the Expression field because whenever we have an LTR field in the RTL direction (such as expressions, emails, URLs, etc.) the placeholder should also be LTR; otherwise it will be confusing for the users.
The code is now formatted with Prettier and eslint. I tried my best to adhere to the formatting guidelines.

@Skaiir I'd appreciate it if you take a look at this.

okaeiz · 2024-07-03T08:35:32Z

Any updates?

Skaiir · 2024-08-15T09:18:37Z

Hey @okaeiz, sorry we haven't been able to be around, unfortunately things got pretty busy pretty fast. I do have some amount of time now so I'll catch up with this today.

Skaiir · 2024-08-21T17:16:47Z

Apologies for the delays it looks like this won't get taken a look at for another week at least. Unfortunately some other priorities came up.

okaeiz · 2024-08-21T20:03:08Z

Take your time. I'll be available for further modifications.

okaeiz · 2024-10-30T08:32:20Z

What is the status of this PR? Should I give up already?

renovate bot and others added 2 commits June 16, 2024 13:48

chore: update dependency @rollup/plugin-commonjs to v26

9120642

Implemented RTL adherence for Action and Selection components

6520e02

okaeiz force-pushed the develop branch 3 times, most recently from 8f337a7 to 6520e02 Compare June 18, 2024 07:33

Added DirectionEntry to the AppearanceGroup

79d38e9

Implemented RTL adherence for Input and Presentation and Containers c…

53980fa

…omponents

Skaiir added the deploy-preview label Jun 19, 2024

Skaiir self-requested a review June 19, 2024 07:58

Skaiir removed the deploy-preview label Jun 19, 2024

Skaiir requested changes Jun 19, 2024

View reviewed changes

fixed some of the reviewer issues

569c889

okaeiz mentioned this pull request Jul 13, 2024

Right-to-Left support for form elements (text field, text area, select, etc.) camunda/camunda-modeler#4306

Open

nikku added the backlog Queued in backlog label Jul 23, 2024 — with bpmn-io-tasks

Skaiir force-pushed the develop branch from bc6a48c to 052b03e Compare August 26, 2024 11:42

nikku self-requested a review October 31, 2024 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTL direction support via properties panel (revised) #1213

RTL direction support via properties panel (revised) #1213

okaeiz commented Jun 18, 2024

okaeiz commented Jun 18, 2024

okaeiz commented Jun 18, 2024

okaeiz commented Jun 18, 2024

Skaiir commented Jun 19, 2024

Skaiir left a comment •

edited

Loading

Skaiir Jun 19, 2024

okaeiz Jun 26, 2024

Skaiir Jun 19, 2024

okaeiz Jun 26, 2024

Skaiir Jun 19, 2024

Skaiir Jun 19, 2024

okaeiz Jun 26, 2024

okaeiz Jun 26, 2024

okaeiz commented Jun 26, 2024

okaeiz commented Jul 3, 2024

Skaiir commented Aug 15, 2024

Skaiir commented Aug 21, 2024

okaeiz commented Aug 21, 2024

okaeiz commented Oct 30, 2024

RTL direction support via properties panel (revised) #1213

Are you sure you want to change the base?

RTL direction support via properties panel (revised) #1213

Conversation

okaeiz commented Jun 18, 2024

okaeiz commented Jun 18, 2024

okaeiz commented Jun 18, 2024

okaeiz commented Jun 18, 2024

Skaiir commented Jun 19, 2024

Skaiir left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okaeiz commented Jun 26, 2024

okaeiz commented Jul 3, 2024

Skaiir commented Aug 15, 2024

Skaiir commented Aug 21, 2024

okaeiz commented Aug 21, 2024

okaeiz commented Oct 30, 2024

Skaiir left a comment •

edited

Loading