-
-
Notifications
You must be signed in to change notification settings - Fork 109
Improve page display to match the screenshot #98
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
Conversation
|
@MasterOdin What do you think, should this be merged before #96? I want to avoid fixing tests twice at any cost. |
|
I'm fine updating my PR after this gets merged. |
|
ping @waldyrious |
be70a51 to
09e4209
Compare
|
@felixonmars Why was I removed it in this PR. |
|
git blame shows that it was added in #20 |
|
It feels like you're missing the point of the command: Note, this does require my fix from #96 or commenting out lines 223-225 to get the color + attribute to work properly on your branch. |
|
@MasterOdin For comparision, this is how it looks after merging your PR: |
|
I would say set I mean, feel free to remove
I can buy complicating writing the display logic, but not sure I buy copy/paste. Testing out master in a terminal window on cmd.exe, macOS iTerm2 using zsh, and Ubuntu bash, when I selected and copied the entire page, there was no extraneous spaces on each line, regardless of background color or not. I suppose you could just skip printing out the content if |
|
@MasterOdin This is what I get in VSCode after copying and pasting the output from (Note: it may seem there is trailing whitespace at the end of all lines, but that's not the case, just the peculiarity of VSCode's highlighting) Bonus: To conclude, with all the changes I proposed (proper highlighting and removing the whitespace), I also don't see the reason to set tldr's background to something special (as basically no other command-line tool does that). The biggest problem I see with setting the background is that it requires adding whitespace. Such approach also bugs multi-line display of a single line, which is very common on 80x24 terminals. |
|
Note: lines 37-74 can be removed since columns and rows size of the terminal is not used anymore. Rows were actually never used, while columns were used only to add spaces. |
|
The background stuff was done in #1 which I guess was because the node client did at the time as well, and has just been carried forward since then. I cannot say whether or not people have been using this option, just that it has existed in tldr for 6 years at this point.
That's the trick right? It's hard to say what end users are doing. While I also don't personally bother setting the background color, that's not to say there's someone out there who isn't. Given that it was added as a set hard-coded option to be a |
|
I think it's more that the node client used to set the background, and the background stuff was added to the python client to make it feature parity with the node client. Can always just remove it here and see if anyone complains and re-address this debate then. |
|
@MasterOdin Can you come to Gitter for a moment? |













Closes #79.
Summary of the changes:
blankcolor removedcprintreplaced withcoloredScreenshot:
