Skip to content

fix(i18n): force install command direction to 'ltr'#1055

Open
BobbieGoede wants to merge 3 commits intonpmx-dev:mainfrom
BobbieGoede:fix/install-command-ltr
Open

fix(i18n): force install command direction to 'ltr'#1055
BobbieGoede wants to merge 3 commits intonpmx-dev:mainfrom
BobbieGoede:fix/install-command-ltr

Conversation

@BobbieGoede
Copy link
Contributor

@BobbieGoede BobbieGoede commented Feb 5, 2026

Resolves #1051

If there are more commands rendered somewhere let me know.

image

@vercel
Copy link

vercel bot commented Feb 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Feb 5, 2026 11:21pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 5, 2026 11:21pm
npmx-lunaria Ignored Ignored Feb 5, 2026 11:21pm

Request Review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 5, 2026

📝 Walkthrough

Walkthrough

The pull request updates the Terminal Install Vue component to add direction attributes to three div elements: the main install command container is set to dir="ltr", and the "Run" and "Create" command comment lines are set to dir="auto". No logic, data flow, event handling, or exported/public entity declarations were changed.

Suggested reviewers

  • danielroe
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description references issue #1051 and acknowledges the changes related to command direction attributes, directly relating to the changeset.
Linked Issues check ✅ Passed The PR successfully adds dir="ltr" to the install command container and dir="auto" to Run/Create command lines, addressing issue #1051's requirement to fix RTL rendering of installation commands.
Out of Scope Changes check ✅ Passed All changes are scoped to adding dir attributes to the Terminal/Install.vue component, directly addressing the linked issue #1051 with no unrelated modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 | 🟡 Minor

The 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:

  1. @types install (line 137): The div with v-for="pm in packageManagers"
  2. Run command (line 172): The div with v-for="pm in packageManagers"
  3. 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 adding dir="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
Copy link

codecov bot commented Feb 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@DDeenis
Copy link
Contributor

DDeenis commented Feb 5, 2026

LGTM 👍

@BobbieGoede BobbieGoede marked this pull request as draft February 5, 2026 22:54
@BobbieGoede
Copy link
Contributor Author

BobbieGoede commented Feb 5, 2026

needs some tweaks
image

@DDeenis what would be the best way to deal with the comments here?

@DDeenis
Copy link
Contributor

DDeenis commented Feb 5, 2026

I would say, ideally they should be auto since they can be Arabic?

@DDeenis
Copy link
Contributor

DDeenis commented Feb 5, 2026

Let's try this - commands always LTR, comments auto

@BobbieGoede
Copy link
Contributor Author

@DDeenis
image

@DDeenis
Copy link
Contributor

DDeenis commented Feb 5, 2026

Nice! That looks really good

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

@DDeenis
Copy link
Contributor

DDeenis commented Feb 5, 2026

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">
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like v-always-ltr?

skaldebane added a commit to skaldebane/npmx.dev that referenced this pull request Feb 6, 2026
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.

Installation commands should have dir="auto" or dir="ltr"

4 participants