Skip to content

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

Merged
danielroe merged 3 commits into
npmx-dev:mainfrom
BobbieGoede:fix/install-command-ltr
Feb 6, 2026
Merged

fix(i18n): force install command direction to 'ltr'#1055
danielroe merged 3 commits into
npmx-dev:mainfrom
BobbieGoede:fix/install-command-ltr

Conversation

@BobbieGoede

@BobbieGoedeBobbieGoede commented Feb 5, 2026

Copy link
Copy Markdown
Contributor

Resolves#1051

If there are more commands rendered somewhere let me know.

image

@vercel

vercelBot commented Feb 5, 2026

Copy link
Copy Markdown
Contributor

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

ProjectDeploymentActionsUpdated (UTC)
npmx.devReadyReadyPreview, CommentFeb 5, 2026 11:21pm
2 Skipped Deployments
ProjectDeploymentActionsUpdated (UTC)
docs.npmx.devIgnoredIgnoredPreviewFeb 5, 2026 11:21pm
npmx-lunariaIgnoredIgnoredFeb 5, 2026 11:21pm

Request Review

@coderabbitai

coderabbitaiBot commented Feb 5, 2026

Copy link
Copy Markdown
Contributor
📝 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 nameStatusExplanation
Description check✅ PassedThe pull request description references issue #1051 and acknowledges the changes related to command direction attributes, directly relating to the changeset.
Linked Issues check✅ PassedThe 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✅ PassedAll 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.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

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

codecovBot commented Feb 5, 2026

Copy link
Copy Markdown

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
Copy Markdown
Contributor

LGTM 👍

@BobbieGoede
BobbieGoede marked this pull request as draft February 5, 2026 22:54
@BobbieGoede

BobbieGoede commented Feb 5, 2026

Copy link
Copy Markdown
ContributorAuthor

needs some tweaks
image

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

@DDeenis

Copy link
Copy Markdown
Contributor

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

@DDeenis

Copy link
Copy Markdown
Contributor

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

@BobbieGoede

Copy link
Copy Markdown
ContributorAuthor

@DDeenis
image

@DDeenis

Copy link
Copy Markdown
Contributor

Nice! That looks really good

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Comment threadapp/components/Terminal/Install.vue Outdated
@DDeenis

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

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
Copy Markdown
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
@danielroe
danielroe added this pull request to the merge queueFeb 6, 2026
Merged via the queue into npmx-dev:main with commit 750eb66Feb 6, 2026
16 checks passed
Sign up for freeto 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"

5 participants

@BobbieGoede@DDeenis@serhalp@danielroe@skaldebane