AlignmentMatrixControl: keep the physical direction in RTL languages#43126
AlignmentMatrixControl: keep the physical direction in RTL languages#43126
Conversation
t-hamano
added
Internationalization (i18n)
|
Size Change: +105 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
Tagging some reviewers who've worked on the Cover block to check that this solution makes sense in that context. |
|
I prepared a wrapper function and added processing by c88a25c2ecc2bcad82775a6c300e5278.mp4 |
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
This is an improvement over the current behavior, so I'm fine with merging. However, I'm not sure if it's a good permanent fix.
The part that I'm worried about is that the visual labels are now mismatched with the value in the component state. It makes me wonder if the RTL handling should be done in a different layer.
Although the AlignmentMatixControl is currently only used in the Cover block, it would be nice to keep them from being tightly coupled. The fix proposed in this PR relies on the fact that the Cover block is converting physical directions (left) to logical directions (start):
gutenberg/packages/block-library/src/cover/style.scss
Lines 129 to 132 in e5154c9
If we imagine a consumer that does not do this, and instead uses the alignment values to do physical positioning like position: absolute; left: 0;, the behavior is quite confusing. If the user selected the dot in the top left corner, you want the component to give you the top left value, regardless of LTR/RTL.
tl;dr — Because AlignmentMatrixControl is dealing with physical direction values (left, right), and there's nothing about the component that suggests it's meant for aligning text, I think it's confusing that it returns flipped values in RTL mode. The matrix dots themselves shouldn't be flipped in RTL. Any RTL flipping, if desired, should be handled by the consumer.
What do you think? In any case it's more of a follow-up and not particularly high priority, so if you think we should merge this PR for the time being, just add a changelog and I can approve 👍
|
Thank you for your very detailed advice 🙂 I have considered the following approach, but is this a radical solution? Fix AlignmentMatrixControl to LTRAdd This will maintain the direction of the grid regardless of the language direction, and labels and values should also show the physical direction. Cover block adjustmentI think that the fixed grid direction will change the existing behavior in the cover block. |
|
Yes, I would say that is the cleanest fix for this problem 👍 I don't know offhand if the |
t-hamano
changed the title
|
✅ In In the cover block, I think the current behavior is not inherently expected. |
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
In the cover block, I think the current behavior is not inherently expected.
Good point. The Cover block is usually used to place text over images, and images don't get flipped in RTL. When you consider a multilingual site that has both LTR/RTL languages, I think it would be desirable to maintain the physical position in either language type.
|
Thanks for the approval! If I may, I would appreciate a review of this PR. |
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for working through this one @t-hamano 👍
From what I can gather this appears to be testing as advertised.
✅ Grid is maintained and tooltips match in Storybook regardless of language direction
✅ On trunk the tooltips didn't match the physical direction of the grid
✅ With this PR's changes applied, the tooltips match the physical direction of the grid
✅ Order of the blocks appear consistent before and after these changes
❓While testing I did notice that the block toolbar is out of position. I'm guessing this isn't related to this PR but it wasn't an issue on trunk. Perhaps just a rebase?
It might also help others coming to this PR with limited history or context if we can get some before/after demos in the PR description.
| Trunk - RTL | This PR - RTL |
|---|---|
 |
 |
I have looked into this and it appears that this is not a problem that is limited to this PR and cover block. 4d2eeb25fcffe86c93cdaa5a4fcb3efd.mp4Perhaps this should be addressed as a separate issue.
I understand, and I will keep that in mind from now on 🙂 |
| "include": [ | ||
| "src/**/*" | ||
| ] | ||
| "include": [ "src/**/*" ] |
There was a problem hiding this comment.
When the latest trunk was merged, unexpected code reformatting was performed.
But since this change was made by the linter, it appears to be OK.
|
@aaronrobertshaw |
Thanks for creating the issue and merging this. 👍 |
3 participants
Fix: #36237
What?
This PR fixes a problem in the RTL language where tooltip labels for theAlignmentMatrixControlcomponent are not displayed correctly.This PR fixes the physical position of the grid in
AlignmentMatrixControlcomponent, regardless of language orientation.Why?
In the RTL language, the grid on which the
AlignmentMatrixControlcomponent is written is inverted left to right.However, tooltip labels do not take language orientation into account, so the left-right labels will be reversed.Therefore, the physical direction indicated by the tooltip is horizontally reversed in the RTL language.
How?
In the variable that defines tooltip labels, I addedisRTLfunction to separate labels.Applied
direction:ltrto a row that is part of a component.In addition, I have added a fix to maintain RTL language behavior in the cover block, which is the only one using this component.
Testing Instructions
Test on the storybook
AlignmentMatrixControlcomponent, confirm that the grid is maintained when the language direction is changed and that the tooltip text also points in the correct direction.Test on the block editor
trunkand open the post / site editor.AlignmentMatrixControlto align to either left or right.rtlwith the following code:rtlwith the following code: