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

Replace emoji with jest-style colors #57

Merged
merged 4 commits into from
Jun 18, 2021
Merged

Conversation

lanwen
Copy link
Contributor

@lanwen lanwen commented Jun 7, 2021

I've experimented with some true-console experience
and tried to implement some jest-like badges.
That's not so distracting as more or less fits into
terminal color palette and resolution. I would say it
also feels a bit more like a professional tool, but that's for sure IMO only.

Just take a look and please tell me WDYT?

image
image

I've experimented with some true-console experience
and tried to implement some jest-like badges.
That's not so distracting as more or less fits into
terminal color palette and resolution. I would say it
also feels a bit more like a professional tool, but that's for sure IMO only.

Just take a look and please tell me WDYT?
@lanwen lanwen marked this pull request as ready for review June 7, 2021 21:22
@sledigabel sledigabel self-assigned this Jun 8, 2021
@sledigabel
Copy link
Contributor

@lanwen thank you, the feedback is pretty positive so far. We'll give it a few more days to think it over. I'll give it a try too see how the interface behaves.

In the meantime if anyone has an opinion on this please speak :-)

@rnorth
Copy link
Collaborator

rnorth commented Jun 16, 2021

FWIW, I quite like it!

@sledigabel
Copy link
Contributor

@lanwen apologies for the delay on this,
I think we're close to merging, I decided to give it a go and it renders a bit differently to me:
image

Not sure why the OK, WARNING and FAIL render as grey. Seems to render differently for you. Could you look into making it black or something clearly visible before merge?

@sledigabel
Copy link
Contributor

Perhaps a colors.Normal() is needed before printing the headers?

@lanwen
Copy link
Contributor Author

lanwen commented Jun 18, 2021

sure, would try with a different color scheme in a terminal - seems it affects colors somehow

@lanwen
Copy link
Contributor Author

lanwen commented Jun 18, 2021

@sledigabel pushed a change - seems that bold broke colors in some terminals:

image

when other looks fine:
image

not sure I can understand now why that happens :)

@sledigabel
Copy link
Contributor

Tested, looks excellent;
Great work @lanwen, thank you for your contribution!

Great spot about the bold prints, I'm guessing the differences came from the terminal themes. Gruvbox seems to clear this out:
image

Copy link
Contributor

@sledigabel sledigabel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modifies the logger look and removes the emojis.

@sledigabel sledigabel merged commit 2a449f5 into Skyscanner:main Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants