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.

Sign up for GitHub

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

RTL direction support via properties panel (revised) #1213

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

RTL direction support via properties panel (revised) #1213

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9120642
chore: update dependency @rollup/plugin-commonjs to v26
renovate[bot] Jun 5, 2024
6520e02
Implemented RTL adherence for Action and Selection components
okaeiz Jun 18, 2024
79d38e9
Added DirectionEntry to the AppearanceGroup
okaeiz Jun 18, 2024
53980fa
Implemented RTL adherence for Input and Presentation and Containers c…
okaeiz Jun 18, 2024
569c889
fixed some of the reviewer issues
okaeiz Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 90 additions & 41 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
"@playwright/test": "^1.43.1",
"@rollup/plugin-alias": "^5.1.0",
"@rollup/plugin-babel": "^6.0.4",
"@rollup/plugin-commonjs": "^25.0.7",
"@rollup/plugin-commonjs": "^26.0.0",
"@rollup/plugin-json": "^6.1.0",
"@rollup/plugin-node-resolve": "^15.2.3",
"@svgr/rollup": "^8.1.0",
Expand Down
61 changes: 61 additions & 0 deletions packages/form-js-editor/src/features/properties-panel/entries/DirectionEntry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
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';

Check failure on line 4 in packages/form-js-editor/src/features/properties-panel/entries/DirectionEntry.js

View workflow job for this annotation

GitHub Actions / Build (ubuntu-20.04, 20) 

Unexpected path "../../../../../form-js-viewer/src/render/context/DirectionContext" imported in restricted zone
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

✅


export function DirectionEntry(props) {
const { editField, field } = props;

const entries = [
{
id: 'direction',
component: Direction,
editField,
field,
isEdited: isSelectEntryEdited,
isDefaultVisible: (field) => field.type === 'default',
},
];

return entries;
}

function Direction(props) {
const { editField, field, id } = props;

Check failure on line 24 in packages/form-js-editor/src/features/properties-panel/entries/DirectionEntry.js

View workflow job for this annotation

GitHub Actions / Build (ubuntu-20.04, 20) 

'id' is assigned a value but never used. Allowed unused vars must match /^_/u
const { setDirection } = useDirection(); // Get the context
okaeiz marked this conversation as resolved.
Show resolved Hide resolved

const debounce = useService('debounce');

const path = ['direction'];

const getValue = () => {
const value = get(field, path, 'ltr');
return value;
};

const setValue = (value) => {
setDirection(value); // Update context
okaeiz marked this conversation as resolved.
Show resolved Hide resolved
return editField(field, path, value || 'ltr');
};

const getOptions = () => [
{
label: 'Left to Right',
value: 'ltr',
},
{
label: 'Right to Left',
value: 'rtl',
},
];

return SelectEntry({
debounce,
element: field,
getValue,
id: 'direction',
label: 'Direction',
setValue,
getOptions,
});
}
2 changes: 2 additions & 0 deletions packages/form-js-editor/src/features/properties-panel/entries/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,5 @@
export { HeadersSourceSelectEntry } from './HeadersSourceSelectEntry';
export { ColumnsExpressionEntry } from './ColumnsExpressionEntry';
export { StaticColumnsSourceEntry } from './StaticColumnsSourceEntry';
export { DirectionEntry } from './DirectionEntry';
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

✅


Check failure on line 43 in packages/form-js-editor/src/features/properties-panel/entries/index.js

View workflow job for this annotation

GitHub Actions / Build (ubuntu-20.04, 20) 

Delete `⏎`
5 changes: 4 additions & 1 deletion packages/form-js-editor/src/features/properties-panel/groups/AppearanceGroup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import { AdornerEntry, GroupAppearanceEntry, LayouterAppearanceEntry } from '../entries';
import { AdornerEntry, GroupAppearanceEntry, LayouterAppearanceEntry, DirectionEntry } from '../entries';

export function AppearanceGroup(field, editField, getService) {
const entries = [
...AdornerEntry({ field, editField }),
...GroupAppearanceEntry({ field, editField }),
...LayouterAppearanceEntry({ field, editField }),
...LayouterAppearanceEntry({ field, editField }),
okaeiz marked this conversation as resolved.
Show resolved Hide resolved
...DirectionEntry({ field, editField, getService }),

Check failure on line 10 in packages/form-js-editor/src/features/properties-panel/groups/AppearanceGroup.js

View workflow job for this annotation

GitHub Actions / Build (ubuntu-20.04, 20) 

Delete `⏎`
];

if (!entries.length) {
Expand Down
16 changes: 12 additions & 4 deletions packages/form-js-viewer/src/render/components/form-fields/Button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,25 @@
import { formFieldClasses } from '../Util';
import { useSingleLineTemplateEvaluation } from '../../hooks';
import { useSingleLineTemplateEvaluation, useService } from '../../hooks';

const type = 'button';

export function Button(props) {
const { disabled, onFocus, onBlur, field } = props;

const { action = 'submit' } = field;

const evaluatedLabel = useSingleLineTemplateEvaluation(field.label || '', { debug: true });

const form = useService('form');
const { schema } = form._getState();

const direction = schema?.direction || 'ltr'; // Fetch the direction value from the form schema
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


return (
<div class={formFieldClasses(type)}>
<div
class={formFieldClasses(type)}
style={{
direction: direction,
fontFamily: 'Vazirmatn, sans-serif',
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

✅

Copy link
Author

Choose a reason for hiding this comment

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

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.

}}>
<button
class="fjs-button"
type={action}
Expand Down
16 changes: 15 additions & 1 deletion packages/form-js-viewer/src/render/components/form-fields/Checkbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Description } from '../Description';
import { Errors } from '../Errors';
import { Label } from '../Label';
import { useService } from '../../hooks';

import { formFieldClasses } from '../Util';

Expand All @@ -23,9 +24,17 @@ export function Checkbox(props) {

const descriptionId = `${domId}-description`;
const errorMessageId = `${domId}-error-message`;
const form = useService('form');
const { schema } = form._getState();
const direction = schema?.direction || 'ltr'; // Fetch the direction value from the form schema

return (
<div class={classNames(formFieldClasses(type, { errors, disabled, readonly }), { 'fjs-checked': value })}>
<div
class={classNames(formFieldClasses(type, { errors, disabled, readonly }), { 'fjs-checked': value })}
style={{
direction: direction,
fontFamily: 'Vazirmatn, sans-serif',
}}>
<Label htmlFor={domId} label={label} required={required}>
<input
checked={value}
Expand All @@ -40,6 +49,11 @@ export function Checkbox(props) {
required={required}
aria-invalid={errors.length > 0}
aria-describedby={[descriptionId, errorMessageId].join(' ')}
style={{
display: 'flex',
justifyContent: direction === 'rtl' ? 'flex-end' : 'flex-start',
fontFamily: 'Vazirmatn, sans-serif',
}}
/>
</Label>
<Description id={descriptionId} description={description} />
Expand Down
Loading
Loading