fix(i18n): force install command direction to 'ltr'#1055
fix(i18n): force install command direction to 'ltr'#1055BobbieGoede wants to merge 3 commits intonpmx-dev:mainfrom
'ltr'#1055Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughThe pull request updates the Terminal Install Vue component to add direction attributes to three div elements: the main install command container is set to Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Terminal/Install.vue (1)
109-133:⚠️ Potential issue | 🟡 MinorThe fix is correct but incomplete — other command sections in this file also need
dir="ltr".The
dir="ltr"attribute correctly addresses the RTL issue for the install command. However, this file renders three additional command types that would have the same problem:
- @types install (line 137): The div with
v-for="pm in packageManagers"- Run command (line 172): The div with
v-for="pm in packageManagers"- Create command (line 217): The div with
v-for="pm in packageManagers"All command blocks should have consistent LTR direction since they're all shell commands in English.
🔧 Proposed fix to add `dir="ltr"` to the remaining command sections
<!-- `@types` package install - render all PM variants when types package exists --> <template v-if="typesPackageName && showTypesInInstall"> <div v-for="pm in packageManagers" :key="`types-${pm.id}`" :data-pm-cmd="pm.id" + dir="ltr" class="flex items-center gap-2 min-w-0" ><div v-for="pm in packageManagers" :key="`run-${pm.id}`" :data-pm-cmd="pm.id" + dir="ltr" class="flex items-center gap-2 group/runcmd" ><div v-for="pm in packageManagers" :key="`create-${pm.id}`" :data-pm-cmd="pm.id" + dir="ltr" class="flex items-center gap-2 group/createcmd" >
🧹 Nitpick comments (1)
app/components/Terminal/Install.vue (1)
166-170: Consider addingdir="ltr"to comment lines as well.The comment lines (e.g.,
# Run locally,# Create project) are also rendered within the terminal and contain English text. For full consistency in RTL locales, these should also be LTR.♻️ Optional: Add `dir="ltr"` to comment lines
<!-- Comment line --> - <div class="flex items-center gap-2 pt-1"> + <div class="flex items-center gap-2 pt-1" dir="ltr"> <span class="text-fg-subtle font-mono text-sm select-none" ># {{ $t('package.run.locally') }}</span > </div><!-- Comment line --> - <div class="flex items-center gap-2 pt-1 select-none"> + <div class="flex items-center gap-2 pt-1 select-none" dir="ltr"> <span class="text-fg-subtle font-mono text-sm"># {{ $t('package.create.title') }}</span>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
LGTM 👍 |
|
@DDeenis what would be the best way to deal with the comments here? |
|
I would say, ideally they should be auto since they can be Arabic? |
|
Let's try this - commands always LTR, comments |
|
Nice! That looks really good |
|
Note - we are waiting for a confirmation from a person who speaks Arabic |
| <span class="w-2.5 h-2.5 rounded-full bg-fg-subtle" /> | ||
| </div> | ||
| <div class="px-3 pt-2 pb-3 sm:px-4 sm:pt-3 sm:pb-4 space-y-1 overflow-x-auto"> | ||
| <div class="px-3 pt-2 pb-3 sm:px-4 sm:pt-3 sm:pb-4 space-y-1 overflow-x-auto" dir="ltr"> |
There was a problem hiding this comment.
This (and other others) might easily get mistakenly "fixed" in the future. Could we leave an explanatory comment on each one? or maybe a custom directive with an expressive name that just sets dir="ltr" could serve the same purpose? https://vuejs.org/guide/reusability/custom-directives
There was a problem hiding this comment.
Maybe something like v-always-ltr?
better fix implemented in npmx-dev#1055


Resolves #1051
If there are more commands rendered somewhere let me know.