Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

console.table text alignment always centered #50117

Closed
p3k opened this issue Oct 10, 2023 · 15 comments · Fixed by #50135
Closed

console.table text alignment always centered #50117

p3k opened this issue Oct 10, 2023 · 15 comments · Fixed by #50135

Comments

@p3k
Copy link

p3k commented Oct 10, 2023

Version

v18.17.1

Platform

Linux nucke 6.2.0-33-generic #33~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu Sep 7 10:33:52 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

internal/cli_table.js

What steps will reproduce the bug?

> console.table(["x", "xxxxxxxxxx"])

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior? Why is that the expected behavior?

┌─────────┬──────────────┐
│ (index) │ Values       │
├─────────┼──────────────┤
│    0    │ 'x'          │
│    1    │ 'xxxxxxxxxx' │
└─────────┴──────────────┘

What do you see instead?

┌─────────┬──────────────┐
│ (index) │    Values    │
├─────────┼──────────────┤
│    0    │     'x'      │
│    1    │ 'xxxxxxxxxx' │
└─────────┴──────────────┘

Additional information

The centered content in the table makes output harder to read and counteracts the idea of using a table to display structured data.

A real-world example:

const response = await fetch("https://api.openopus.org/composer/list/search/straus.json");
const { composers } = await response.json();
console.table(composers[0]);
┌───────────────┬─────────────────────────────────────────────────────────────────┐
    (index)                                 Values                              
├───────────────┼─────────────────────────────────────────────────────────────────┤
      id                                     '171'                              
     name                                  'Strauss'                            
 complete_name                         'Richard Strauss'                        
     birth                               '1864-01-01'                           │
│     death     │                          '1949-01-01'                           │
│     epoch     │                         'Late Romantic'                         │
│   portrait    │ 'https://assets.openopus.org/portraits/29972276-1568084951.jpg' │
└───────────────┴─────────────────────────────────────────────────────────────────┘

It is also not clear why the Node.js implementation differs from those in current browsers:

Edge:
image

Firefox:
image

@MrJithil
Copy link
Member

MrJithil commented Oct 10, 2023

Seems interesting. As per the MDN docs, the table is left aligned. https://developer.mozilla.org/en-US/docs/Web/API/console/table . So, we need a fix here.

However, had a look into the current implementation, and If we can expose a align prop to console.table , then we could allow to use either center, left or right

Alignment Preview
center Screenshot 2023-10-11 at 12 57 24 am
left (default) Screenshot 2023-10-11 at 1 12 26 am
right Screenshot 2023-10-11 at 12 55 37 am

Need a discussion and happy to fix/add feature this.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 10, 2023

Yeah I am +1 this as we use usually table() to show the results in tinybench. And because it is centered, it looks unreadable.

Also configuring the the alignment makes sense, as usually you want to align text to the left and numbers to the right (see excel).

But aligning is not documented in mdn. Are we allowed to deviate?

@Aslemammad

@MrJithil
Copy link
Member

Yeah I am +1 this as we use usually table() to show the results in tinybench. And because it is centered, it looks unreadable.

Also configuring the the alignment makes sense, as usually you want to align text to the left and numbers to the right (see excel).

But aligning is not documented in mdn. Are we allowed to deviate?

@Aslemammad

We should not be deviated from the JS standards.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 11, 2023

There is no real standard

https://console.spec.whatwg.org/#table

@aduh95
Copy link
Contributor

aduh95 commented Oct 12, 2023

We should also think about RTL languages, it's been years since the last time I've opened Excel but I'm pretty sure it aligns text to the left only for LTR text. The upside of using center align is that we don't need to think about it.

console.table([
    'طنجة',
    'تطوان',
    'القصر الكبير',
    'Larache',
    'الفنيدق'
])

@MrJithil
Copy link
Member

As per the browser implementations, both RTL and LTR texts are left aligned.

Safari
Screenshot 2023-10-12 at 5 59 50 pm

Chrome
Screenshot 2023-10-12 at 6 05 32 pm

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 12, 2023

Does node have a bidi implentation? I used years ago bidi implentation by @ashensis in jspdf
https://github.com/parallax/jsPDF/blob/5d09af9135a2fe049c7d3c8b95df280d22e4a6db/src/libs/bidiEngine.js#L2655

Also what happens if you use arabic ligatures? Like لا ?
Do we present them also correct and get the right text width? Or what about short vowels, like the Kasra?

This can open up a big hornet nest of issues...

@MrJithil
Copy link
Member

Does node have a bidi implentation? I used years ago bidi implentation by @ashensis in jspdf
https://github.com/parallax/jsPDF/blob/5d09af9135a2fe049c7d3c8b95df280d22e4a6db/src/libs/bidiEngine.js#L2655

Also what happens if you use arabic ligatures? Like لا ?
Do we present them also correct and get the right text width? Or what about short vowels, like the Kasra?

This can open up a big hornet nest of issues...

Is this a question to this PR or a general discussion? Please feel free to start a new one in the discussion tab, or raise a new issue with your questions please. So, reviewers will not be confused.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 12, 2023

It is not a question and not a general discussion. It is an answer to @aduh95 .

@aduh95
Copy link
Contributor

aduh95 commented Oct 12, 2023

As per the browser implementations, both RTL and LTR texts are left aligned.

Did you try when the system lang is set to a RTL language?

@aduh95
Copy link
Contributor

aduh95 commented Oct 12, 2023

It is not a question and not a general discussion. It is an answer to @aduh95 .

There are a lot of interrogation points in your comment, which I think is why it was interpreted as a question rather than a answer. FWIW I think we simply pass Unicode characters to stdout and let the terminal deal with it (I'm just guessing here, I haven't looked at the code).

@MrJithil
Copy link
Member

MrJithil commented Oct 12, 2023

As per the browser implementations, both RTL and LTR texts are left aligned.

Did you try when the system lang is set to a RTL language?

No. But, I tried now before answering this. So both RTL and LTR its left align.

‏لقطة الشاشة 2023-10-12 في 9 48 38 م

@aduh95
Copy link
Contributor

aduh95 commented Oct 12, 2023

For completeness, in DevTools using a RTL locale:
screenshot of the Chromium DevTool set to arabic showing the text left-aligned

But it doesn't really matter what the browsers are doing, we don't have to copy their behavior because this does not impact interoperability. IMO the browser behavior is "wrong", Node.js behavior is "better", but of course depending on your use-case the browser behavior may or may not make more sense. I'd love to har thoughts about this from folks using RTL languages btw.

@MrJithil
Copy link
Member

Make sense. Sure. Let's wait for RTL users.

@p3k
Copy link
Author

p3k commented Oct 12, 2023

Just to understand where this is going: does Node.js already output its prompt on the right side when started with an RTL locale? Or are you trying to correctly display RTL languages in a tabular output while the Node.js REPL is not capable of RTL output, anyway?

nodejs-github-bot pushed a commit that referenced this issue Oct 28, 2023
PR-URL: #50135
Fixes: #50117
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
PR-URL: nodejs#50135
Fixes: nodejs#50117
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this issue Nov 11, 2023
PR-URL: #50135
Fixes: #50117
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
UlisesGascon pushed a commit that referenced this issue Dec 11, 2023
PR-URL: #50135
Fixes: #50117
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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 a pull request may close this issue.

4 participants