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

Consider wunderbar #157

Closed
paulirish opened this issue Mar 10, 2018 · 8 comments
Closed

Consider wunderbar #157

paulirish opened this issue Mar 10, 2018 · 8 comments

Comments

@paulirish
Copy link
Owner

https://github.com/gribnoysup/wunderbar

image

might be nice instead of our barchart hax. but i'm not sure.

@denar90
Copy link
Collaborator

denar90 commented Mar 10, 2018

Looks pretty, I'll play with it. Should we consider migration after/during #158 ?

@denar90
Copy link
Collaborator

denar90 commented Mar 10, 2018

Looks interesting, not something we used to see:

image

Notes:

  1. Just results of POC playground
  2. Width of bars can be bigger
  3. Colors are configurable

Open questions are:

  • will it be better from UX perspective of view?
  • In another hand, we should not think about supporting our fork of chart...

@paulirish
Copy link
Owner Author

ah nice job giving this a shot.

While I do like the timescale at the bottom, and the colors... I don't think it is a big improvement otherwise. So perhaps its fine to stick with what we got. :)

@gribnoysup
Copy link
Contributor

gribnoysup commented Mar 20, 2018

Wunderbar almost made a full circle, haha 😸I made Wunderbar because I needed something flexible to print data from the PWMetrics and here you were evaluating the possibility to use it.

@paulirish @denar90 first of all, thank you for even considering it, I'm stoked! 😍 Because of that I really want to try to convince you to use it once more. If you are still not feeling like it or just find this comment annoying, feel free to ignore it and sorry in advance!

  • I noticed that you are supporting node >= 6 and Wunderbar was supporting node >= 8. This is not an issue anymore, I fixed it in a recent release 🎉

  • As @denar90 mentioned, one of the benefits of using Wunderbar is that you don't need to maintain cli-chart fork anymore.

  • The second improvement compared to current implementation is that Wunderbar uses partial block symbols to print charts, that means that even small differences in values will be visible on the chart. This wasn't documented well before 😅

  • @denar90 I noticed your concern about changing the UI, so I experimented a little with replacing cli-chart with Wunderbar in my fork and here how it can look in comparison with the current chart. Instead of using default Wunderbar output, I am using building blocks from the library to keep the current PWMetrics UI, but with the slightly polished look.

comparison

I will also submit a PR that you can evaluate as a possible implementation. I'm open to feedback and will gladly change it in any way you want, even if some reasonable upstream changes in the library are needed.

Again, if you still don't want to use the library, feel free to close that PR right away and thank you for considering Wunderbar 🙏

@denar90
Copy link
Collaborator

denar90 commented Mar 24, 2018

@gribnoysup thank you for so grounded and deep investigation! Looks really good! Do you think it will also be fine for windows users? We had some issues with chart behavior before...

@gribnoysup
Copy link
Contributor

gribnoysup commented Mar 25, 2018

Good point! Speaking only from a codebase perspective I would say it should work well, but I didn't have the chance to test on windows before 😅

Give me a few hours to bootstrap a VM; I'll check how it works and will report back!

@gribnoysup
Copy link
Contributor

gribnoysup commented Mar 25, 2018

@denar90 I ran my experimental branch in Windows 10 cmd and PowerShell, and here is how it looks:

virtualbox_windows 10 x64 _25_03_2018_20_21_12

It seems that only half box and full box symbols are supported by win systems out of the box; I'll fix this ASAP. Chart precision wouldn't be as good as on other platforms, but it will still be a step forward from the current version 👍

I also noticed that checkmark for the successful run is not printed properly. Instead of the checkmark, you see [?] symbol. To avoid that, you can also fall back to other symbols on win32 platforms, here is an example in the library called log-symbols. I'll be glad to submit a PR with the fix for that also BTW 😸

@denar90
Copy link
Collaborator

denar90 commented Mar 30, 2018

fixed in #161

@denar90 denar90 closed this as completed Mar 30, 2018
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

No branches or pull requests

3 participants