Added rtl attribute to scripts - #151
Conversation
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on josiahwall).
components/language-chooser/common/find-language/findLanguageInterfaces.ts line 66 at r1 (raw file):
const info = locale.getTextInfo?.() ?? (locale as any).textInfo; return info?.direction === "rtl"; }
Sorry for the churn. After looking to this more, I'm now convinced we need to pass in the script here and look up und-{script}.
Otherwise, with something like Uzbek, Afghanistan, Latin, you get Latin script, RTL.
(Let's add that as a comment on one reason we want to implement it this way. It makes things based solely on the Script, and that is what we are setting the property on.)
f6c0333 to
0004a94
Compare
josiahwall
left a comment
There was a problem hiding this comment.
@josiahwall made 1 comment.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on andrew-polk).
components/language-chooser/common/find-language/findLanguageInterfaces.ts line 66 at r1 (raw file):
Previously, andrew-polk wrote…
Sorry for the churn. After looking to this more, I'm now convinced we need to pass in the script here and look up und-{script}.
Otherwise, with something like Uzbek, Afghanistan, Latin, you get Latin script, RTL.
(Let's add that as a comment on one reason we want to implement it this way. It makes things based solely on the Script, and that is what we are setting the property on.)
Done.
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 2 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on josiahwall).
components/language-chooser/common/find-language/findLanguageInterfaces.ts line 73 at r2 (raw file):
// (since Arabic script is rtl). However, uz-AF's script is Latin, so // the output will end up being a Latin script (which is not rtl) with // the rtl attribute set to true.
As we discussed, let's tweak this comment a bit. My understanding is that it isn't totally accurate to say uz-AF script is Latin but rather that it is ambiguous and we need to ensure that if the user selected Latin, that we export the correct rtl value to match that.
components/language-chooser/common/find-language/findLanguageInterfaces.ts line 75 at r2 (raw file):
// the rtl attribute set to true. export function isRTLScript(scriptCode: string): boolean { const locale = new Intl.Locale(`und-${scriptCode}`).maximize();
We also discussed the good point that you raised that maximize probably doesn't really make sense here now.
0004a94 to
1f69fb7
Compare
josiahwall
left a comment
There was a problem hiding this comment.
@josiahwall made 2 comments.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on andrew-polk).
components/language-chooser/common/find-language/findLanguageInterfaces.ts line 73 at r2 (raw file):
Previously, andrew-polk wrote…
As we discussed, let's tweak this comment a bit. My understanding is that it isn't totally accurate to say uz-AF script is Latin but rather that it is ambiguous and we need to ensure that if the user selected Latin, that we export the correct rtl value to match that.
Done.
components/language-chooser/common/find-language/findLanguageInterfaces.ts line 75 at r2 (raw file):
Previously, andrew-polk wrote…
We also discussed the good point that you raised that
maximizeprobably doesn't really make sense here now.
Done.
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on josiahwall).
The scripts now have a boolean attribute that tells whether the specific script is written right to left or left to right.
This change is