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

[fix] Do not use colors if the terminal doesn't support them #659

Open
wants to merge 1 commit into
base: stretch-unstable
from

Conversation

@maniackcrudelis
Copy link
Contributor

commented Feb 21, 2019

The problem

YunoHost/issues#1309

Solution

Fix YunoHost/issues#1309
Check if the terminal can use colors.

PR Status

Tested, can be reviewed.

How to test

...

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
#
# [internal]
#
are_colors_supported () {

This comment has been minimized.

Copy link
@alexAubin

alexAubin Feb 21, 2019

Member

Can't we optimize this a bit and check this once, globally, rather than every time we run a ynh_print_* ?

This comment has been minimized.

Copy link
@maniackcrudelis

maniackcrudelis Feb 21, 2019

Author Contributor

Not sure we would save any time.
That would means runs that helper at the beginning of any script. Or even when the file is sourced. Do we want to do that ?

@Psycojoker

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

If it's only to fix the webadmin, we can fix that like I did in YunoRunner using this code https://github.com/YunoHost/yunorunner/blob/master/static/js/app.js#L13 and we'll even get pretty colours 😋

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

That's a function that replace bash colors by colors supported by java script ?

@Psycojoker

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

(by html) yes.

@Psycojoker

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

That's how jobs are colorized on individual jobs pages.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Considering that my PR will remove colors for the admin JS. It could be indeed better to adapt colors instead

@alexAubin alexAubin changed the title Do not use colors if the terminal doesn't support them [fix] Do not use colors if the terminal doesn't support them Feb 24, 2019

@alexAubin

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

So uh should we proceed with integrating https://github.com/YunoHost/yunorunner/blob/master/static/js/app.js#L13 instead ?

@Psycojoker

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

That would made more sens yes and also brings colors to the admin UI (hoping it won't make things unreadable)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.