tldr-pages / tldr-python-client

:snake: Python command-line client for tldr pages 📚
https://pypi.org/project/tldr/
MIT License
599 stars 95 forks source link

Do not specify default color by default for white text #95

Closed MasterOdin closed 4 years ago

MasterOdin commented 4 years ago

In the code, tldr defines default colors to use for all attributes: https://github.com/tldr-pages/tldr-python-client/blob/c10b95aa5c6549361d6e986798d42245b79a08d9/tldr.py#L183-L190

As pointed out in #94, these look terrible on non-dark backgrounds as white does not show up well. This is due to the specifying of the white color by default for a number of elements. However, instead of specifying this white default, why not just leave it blank and use the user set text color within their editor? This is probably what the user wants anyway by default and is somewhat guaranteed to look fine for them.

The actual code change here would be to pass None back as the first argument of the tuple from of_colors.

zlatanvasovic commented 4 years ago

Do you think this chang should be implemented in #94?

MasterOdin commented 4 years ago

I think that this would render much of #94 not needed, but no, I do not think it should be in the same PR.

k4j8 commented 4 years ago

Agreed, if we can use the terminal default colors that would be ideal and eliminate the need to suggest a workaround. I'll try it tonight and confirm it works for me then cancel #94.

k4j8 commented 4 years ago

Looks good after setting all of the white colors to 'None'.

Black on light yellow (with all white colors set to none)

none for white - black on light yellow

Black on white (with all white colors set to none)

none for white - black on white

Solarized light (with all white colors set to none)

none for white - solarized light

Not sure if this is the best way to do it, but setting the default color to None results in defaulting to user_value, if specified. So instead, I set it to the string 'none' and then added a line to change it to None.

If you agree, I can submit a new PR and cancel #94.

diff --git a/tldr.py b/tldr.py
index a2c493e..22e4a77 100755
--- a/tldr.py
+++ b/tldr.py
@@ -181,12 +181,12 @@ def get_page(command, remote=None, platform=None):

 DEFAULT_COLORS = {
-    'blank': 'white',
-    'name': 'white bold',
-    'description': 'white',
+    'blank': 'none',
+    'name': 'none',
+    'description': 'none',
     'example': 'green',
     'command': 'red',
-    'parameter': 'white',
+    'parameter': 'none',
 }

 # See more details in the README:
@@ -212,6 +212,7 @@ def colors_of(key):
         user_value = None
     values = user_value or DEFAULT_COLORS[key]
     values = values.split()
+    values = [None if x == 'none' else x for x in values]
     return (
         values[0] if len(values) > 0 else None,
         values[1] if len(values) > 1 and values[1].startswith('on_') else None,
MasterOdin commented 4 years ago

I would prefer to just go with #96 which improves things a lot, and also actually lets users do stuff like set more than just a font color. I think also making it so just setting the value to "" (or unset) gives you default colors is probably more intuitive than special casing the word "none".

k4j8 commented 4 years ago

Sorry, I didn't see #96. Yes, that looks much better. I just tried it and it works great after setting the white colors to an empty string. Thanks!