-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat(utils): implement a new design for stdout #206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it out for our example app (npx nx run cli:run-collect
), added a few visual notes. I'm not sure why the bold formatting isn't working properly in the audits section, since the code is the same as my PoC 😕
I got the feeling cliui
was quite buggy in this, so maybe we could also consider some alternative package (maybe table-layout?) or even write it ourselves (all we need is the column alignment and word wrapping).
packages/utils/src/lib/__snapshots__/report-to-stdout.spec.ts.snap
Outdated
Show resolved
Hide resolved
packages/utils/src/lib/__snapshots__/report-to-stdout.spec.ts.snap
Outdated
Show resolved
Hide resolved
2d8eccb
to
6b84561
Compare
All CLI packages do strange things. I left the current packages but rendered the full report one time. Do not use
in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran it on Windows in VSCode with light theme. Looking good! 🌈
In-scope comment
- While a preceding empty line is nice for the section, it should visually belong to the text below so I wouldn't put a new line right after it. In the original design, the preceding line is bigger than the gap after the title as well.
OOS
- Is there any way of wrapping words to a new line so that they aren't cut off like that?
- To be continued in Ensure text wrapping in stdout #209
- The footer line is missing a whitespace (has been ever since I joined) and I would really like to see that fixed. → Won't fix atm.
- This is a suggestion for both MD and stdout - can we sort the output e.g. based on number of errros, then warnings? The user now has to skim through it seemingly with no logical ordering.
- To be continued in Sorting audits in stdout and MD format #210
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes, the output looks really good now 👍
I have suggestions regarding code style. But the functionality itself is now working as I would expect and the sanitized snapshot is now much more easy to understand as well.
@Tlacenka While I agree with your comments, I think changes to the original design should handled as another issue, so we don't bloat this PR. These changes are already a big improvement over the formatting we have in
Yes, that bugs me as well 😬 Unfortunately, it looks like But I do agree it would improve the output a lot, many of the audit titles won't fit on one line, so it's not really an edge case.
This is a weird one. The space is actually there, it's just that some terminals don't render it after the heart. E.g. my VSCode integrated terminal renders it, but my regular Linux terminal doesn't (same as your Windows terminal) 😕
Yes, I can see that would be useful. Could you create a separate issue for that? I'm not sure what the best ordering would be (could be weight, score, alphabetical 🤷♂️), that's something we can discuss during refinement.
Good catch 👍 In my PoC, I put an extra blank line before the title (so it's 2 blank lines before and 1 after). That would be an easy improvement, I believe. |
I added the extra line before the titles. I agree; this is more readable. I did minor refactoring. About whitespace after ❤️. I tried to change to another symbol. @Tlacenka, could you check it out? If it is OK, I'll add some changes to the test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
67225ca
to
493946b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some small comments, but over all I love the improvements! 🏆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3
Closes #190