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

update style to get rid of emojis #26

Closed
wants to merge 1 commit into from
Closed

update style to get rid of emojis #26

wants to merge 1 commit into from

Conversation

KyleAure
Copy link

@KyleAure KyleAure commented Oct 15, 2020

Testing out if there is a way we can update the styling as on different browsers / operating systems the emojis are not mono-spaced which results in a poor looking output.

@github-actions
Copy link

Unit Test Results

  1 files  ±0    1 suites  ±0   0s ⏱️ ±0s
24 tests ±0  24 ✔️ ±0  0 💤 ±0  0 ✖️ ±0 

results for commit 404b98b ± comparison against base commit 3fefb1a

@KyleAure
Copy link
Author

Unit Test Results

Metadata Test Results Test Suites
1 files  ±0
1 suites ±0
0s duration ±0s
24 tests ±0  
+ 24 succ ±0  
# 0 skip ±0  
- 0 fail ±0
1 suite ±0  
+ 1 succ ±0  
# 0 skip ±0  
- 0 fail ±0
! 1 error

Proposed style

@KyleAure KyleAure marked this pull request as draft October 15, 2020 02:21
@EnricoMi
Copy link
Owner

A nice alignment is really hard, you can tell that from the code.

Can you paste screenshots with your browser / OS of these test results please?

Thanks

@KyleAure
Copy link
Author

Mac OS
Firefox

Screen Shot 2020-10-15 at 11 16 19 AM

@EnricoMi
Copy link
Owner

Can you screenshot these results please: horovod/horovod#2371 (comment) and
https://github.com/horovod/horovod/runs/1255050458? They are a bit more challenging than those in this PR.

@KyleAure
Copy link
Author

Screen Shot 2020-10-15 at 11 24 56 AM
Screen Shot 2020-10-15 at 11 25 19 AM

@KyleAure
Copy link
Author

Also just wanted it to be noted that I did not intend on opening a PR... I was just investigating this repo and needed to push a branch to do a test build. My hand instinctively went to the Open Pull Request button. I can close this PR if you want to discuss this more in a issue.
Sorry!

@EnricoMi
Copy link
Owner

No worries about this being a PR, we can keep discussing here.

I think the problem are not the emojis. They are not monospaced, yes, but all other characters and whitespaces are neither. And since the emojis are in the second and third line at the same position, the misalignment is not due to them but the 1 (which is smaller on your environment than all other digits) and the - (which is smaller than the + and ±). Lets try the following layouts:

@EnricoMi
Copy link
Owner

EnricoMi commented Oct 16, 2020

Unit Test Results

   521 files   -      60     521 suites    - 60   4h 24m 19s ⏱️  -  5m 23s
   508 tests +       1     482 ✔️ +    1       26 💤 ±    0  0 ✖️ ±0 
9 876 runs   - 1 190  7 873 ✔️  - 896  2 003 💤  - 294  0 ✖️ ±0 

results for commit 9b3b2cf7 ± comparison against base commit 9aaeb06f

@EnricoMi
Copy link
Owner

Unit Test Results

       𝟻𝟸𝟷 files   -              𝟼𝟶      𝟻𝟸𝟷 suites    - 𝟼𝟶   𝟺h 𝟸𝟺m 𝟷𝟿s ⏱️  -  𝟻m 𝟸𝟹s
       𝟻𝟶𝟾 tests +                   𝟷         𝟺𝟾𝟸 ✔️ +            𝟷               𝟸𝟼 💤 ±            𝟶  𝟶 ✖️ ±𝟶 
𝟿 𝟾𝟽𝟼 runs   - 𝟷 𝟷𝟿𝟶  𝟽 𝟾𝟽𝟹 ✔️  - 𝟾𝟿𝟼  𝟸 𝟶𝟶𝟹 💤  - 𝟸𝟿𝟺  𝟶 ✖️ ±𝟶 

results for commit 9b3b2cf7 ± comparison against base commit 9aaeb06f

@EnricoMi
Copy link
Owner

Can you screenshot those to comments above?

@EnricoMi
Copy link
Owner

Unit Test Results

       𝟻𝟸𝟷 𝚏𝚒𝚕𝚎𝚜  -               𝟼𝟶     𝟻𝟸𝟷 𝚜𝚞𝚒𝚝𝚎𝚜    - 𝟼𝟶   𝟺𝚑 𝟸𝟺𝚖 𝟷𝟿𝚜 ⏱️  -  𝟻𝚖 𝟸𝟹𝚜
       𝟻𝟶𝟾 𝚝𝚎𝚜𝚝𝚜 +                   𝟷         𝟺𝟾𝟸 ✔️ +            𝟷               𝟸𝟼 💤 ±            𝟶  𝟶 ✖️ ±𝟶 
𝟿 𝟾𝟽𝟼 𝚛𝚞𝚗𝚜        - 𝟷 𝟷𝟿𝟶  𝟽 𝟾𝟽𝟹 ✔️  - 𝟾𝟿𝟼  𝟸 𝟶𝟶𝟹 💤  - 𝟸𝟿𝟺  𝟶 ✖️ ±𝟶 

results for commit 9b3b2cf7 ± comparison against base commit 9aaeb06f

@KyleAure
Copy link
Author

Screen Shot 2020-10-19 at 10 06 49 AM

Sorry for the late reply.

@EnricoMi
Copy link
Owner

That looks even worse now. I think your earlier screenshot does not look too bad.

I was thinking about adding an optional pure monospace version. What do you think?

@EnricoMi
Copy link
Owner

Unit Test Results

  521 files -    60      521 suites - 60     4h 24m 19s duration -5m 23s
  508 tests +     1      482 failed +  1       26 skipped ±  0    0 errors ±0 
9 876 runs  - 1 190    7 873 failed -896    2 003 skipped -294    0 errors ±0 

results for commit 9b3b2cf7 ± comparison against base commit 9aaeb06f

@KyleAure
Copy link
Author

Looks better, but there is still a lot of data with little context surrounding it. It is hard to see what is important.
Perhaps tables would be a better solution?

Screen Shot 2020-10-23 at 9 44 51 AM

@EnricoMi
Copy link
Owner

What about such a monospace table:

521 suites (-60) in 4h 24m 19s (-5m 23s)
508 tests   (+1)
482 success (+1)
 26 skipped (±0)
  0 failed  (±0)
9 876 runs    (-1 190)
7 873 success (-  896)
2 003 skipped (-  294)
    0 failed  (±    0)
results for commit e826078 ± comparison against base commit 3fefb1a

and without runs:

521 suites (-60) in 4h 24m 19s (-5m 23s)
508 tests   (+1)
482 success (+1)
 26 skipped (±0)
  0 failed  (±0)
results for commit e826078
± comparison against base commit 3fefb1a

@KyleAure
Copy link
Author

Yes that looks much better and will be consistent between systems/browsers. One last thing I would suggest would be to add a comma or period to separate the thousands place

9 876 -> 9,876
-- OR --
9 876 -> 9.876

Different depending where you are in the world, but I think generally everyone is comfortable with the comma. Americans will probably be confused by a period ;)

@EnricoMi
Copy link
Owner

I will create an issue to track adding that monospace comment.

For the thousand punctuation . and ,, there are always regions in the world that are confused by any of them. Germans are definitively confused by the ,. That is why I went for the as this is even a PUNCTUATION SPACE, which is standardized as the same size as a .. I could use a THIN SPACE or even a HAIR SPACE (not in monospace of course), which are smaller to make it more readable. Not sure.

@EnricoMi
Copy link
Owner

EnricoMi commented Nov 3, 2020

Thanks for the discussion. I have created issue #46.

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 this pull request may close these issues.

None yet

2 participants