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

fix rtl direction for components #1381

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

Sebastian-ubs
Copy link
Contributor

@Sebastian-ubs Sebastian-ubs commented Dec 11, 2024

This change is Reviewable

This is about fixing correct alignment of ui elements in a right to left layout, not direction of the text, which will be determined by the script of the text itself.

Components fixed

  • Book chapter control: arrow buttons navigation, alignment of the history icon, direction of the popover content, align popover position start vs end
  • Book selector: direction, spacing: tw-ml -> tw-ms
  • Inventory: direction of filter comboboxes
  • MultiSelectCombobox: direction, spacing
  • Result viewer: direction
  • scroll group selector: direction
  • tab nav search: direction
  • chapter range selector: direction of combobx, spacing
  • checklist: spacing
  • combobox: spacing
  • search: alignment of icons + no resizing + fix ring color by removing unneeded duplication of classNames (8b6a9e3)
  • spinner: animation direction -- is it valid to spin anti clockwise in a rtl language? https://github.com/paranext/ux/issues/168#issuecomment-2523558709
  • dialog: mentioned the current bug
  • dropdown: mentioned the current bug
  • switch: alignment of the circle
  • toggle group: direction
  • sidebar: direction + flip icon + (together with Jolie) mark our code comments

Example: BCV

Before
image

After
image

@Sebastian-ubs Sebastian-ubs changed the title fix rtl fix rtl direction for components Dec 11, 2024
@Sebastian-ubs Sebastian-ubs marked this pull request as ready for review December 11, 2024 17:12
@merchako
Copy link
Contributor

I feel like there should be a way of getting direction keyboard events like "RightArrow" and "LeftBracket" but as text-direction-aware abstractions like "EndArrow" and "StartBracket". CSS already does this (e.g. padding-start). Anybody know of a way to do it with the Javascript API?


I saw @slingk 's PR to add text direction awareness to a bunch of components and thought, golly, a JS core feature like like this could save us a lot of code. https://github.com/paranext/paranext-core/pull/1381/files

ChatGPT seems to think any component should be able to get text direction from an element using

const computedStyle = window.getComputedStyle(targetElement);
    const direction = computedStyle.direction; // 'ltr' or 'rtl'

Might we use this approach for components to get text direction without needing to pass it as a parameter?

  • Or perhaps this wouldn't work if the component doesn't already contain a targetElement to grab the direction info from.
  • Or perhaps we need to manually pass down direction since some elements with direction A may contain other elements with direction B.

Convo in Discord

Copy link
Member

@lyonsil lyonsil 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 picking up the work to make these controls work left-to-right and right-to-left. This is very helpful!

Unfortunately, I don't think we want to explicitly have a direction prop to determine ltr or rtl. HTML already defines a mechanism for specifying this, and creating a separate prop to indicate the same idea can cause conflicts/confusion. For React controls, it would be helpful to create a hook that uses getComputedStyle as Alex indicates above. We need to make sure not to call getComputedStyle on every render as it isn't "free", but allowing the controls to call a hook to get the direction will standardize where/how we get it safely.

Reviewable status: 0 of 44 files reviewed, 1 unresolved discussion (waiting on @Sebastian-ubs)


lib/platform-bible-react/dist/index.d.ts line 211 at r1 (raw file):

	handleSubmit: (scrRef: ScriptureReference) => void;
	/** Text and layout direction */
	direction?: "rtl" | "ltr";

We should not be adding direction props to all of our controls. I'm just marking the first one as a block, but the same thought applies to all the later ones. More thoughts in the overall comments for this PR.

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.

3 participants