-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Add dark css for the Help and IPython Console plugins #8086
Conversation
Hello @dalthviz! Thanks for updating the PR.
Comment last updated on October 28, 2018 at 12:40 Hours UTC |
That's the Epic tracking everything that needs to be done to get the dark theme fully ready to go, so we don't want to close that just yet. I've created an issue specifically for the help pane, and edited your post above to resolve that instead. EDIT; Sorry, I didn't see you didn't say "Fixes" there...I already created the issue so I added it there, but my mistake. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dalthviz !
Its not clear what's in scope for this PR, so I'll summarize my comments here and then either implement them here, or open a new issue, add them to #8087 , etc.
- The "Usage" screen shown whenever Spyder is opened still has the light theme applied
- The syntax highlighting has a light background and mostly a light-background syntax scheme, but function calls and assignments, etc. are picked up by the text -> white rules and are thus invisible.
- I suggest a light-medium grey for the code, admonition, signature, etc. boxes if you want to keep using a light theme, since the current very light gray is glaringly bright given the rest of the dark theme. Alternatively, you could use a dark syntax theme and straight black or medium-dark grey for the boxes.
- Links are rather dark and hard to read; like the Editor, I suggest a lighter shade of blue/cyan
@dalthviz, please apply the changes in this PR to the IPython console, i.e. to the info_widget of each client, which uses the same CSS as the Help's rich widget. |
@ccordoba12 @CAM-Gerlach a new preview: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking much better! A few additional comments:
- Can you make the white/light interstitial color (when you trigger introspection and the docstring retrieval and rendering is in progress) the normal dark-theme BG color as well? Its very jarring for it to suddenly flash to white, then back to dark.
- Could you make links, titles etc. (stuff colored blue) the same lighter blue color as the header bar? They're still a little hard to read.
- I definitely prefer the look of the code blocks in this version, but now the contrast between the code and the background is getting a little low. If it isn't too much work, would you consider instead making the code the standard Spyder dark theme and the code block background a medium-dark gray (darker than the current, but lighter than the background grey)?
Thanks!
@ccordoba12 The test failure is the same |
Ok, thanks for the suggestion @CAM-Gerlach! I'm preparing a PR right now to fix that. |
@dalthviz, please rebase in top of master to get the latest fixes to our tests. |
@dalthviz That looks really sharp and clean! Fantastic! Only thing is I prefer the colors of the title bar and body of the "Usage" and "An error occurred starting the kernel" boxes to be swapped, as you had in your previous GIF—it looks kind of bottom-heavy and unexpected as is, since generally in UIs one sees the stronger color as part of the title + surrounding frame, while the "body" (e.g. TEST, usage instructions) is more neutral. |
I instructed @dalthviz to follow this https://bootswatch.com/darkly/ so that's why it looks so good now!
That's debatable, but I prefer to follow the Darkly theme as it was designed, so I'm going to merge this one as it is for now. Of course, things can changed latter if others agree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @dalthviz! I left some minor comments, then this will be ready.
spyder/app/mainwindow.py
Outdated
@@ -576,12 +576,19 @@ def setup(self): | |||
logger.info("Applying theme configuration...") | |||
ui_theme = CONF.get('color_schemes', 'ui_theme') | |||
color_scheme = CONF.get('color_schemes', 'selected') | |||
from spyder.plugins.help.utils.sphinxify import CSS_PATH, DARK_CSS_PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this import to the top of this file
@@ -76,6 +77,10 @@ | |||
dependencies.add("matplotlib", _("Display 2D graphics in the IPython Console"), | |||
required_version=MATPLOTLIB_REQVER, optional=True) | |||
|
|||
# CSS style | |||
PLUGINS_PATH = get_module_source_path('spyder', 'plugins') | |||
CSS_PATH = osp.join(PLUGINS_PATH, 'help', 'utils', 'static', 'css') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need for these two lines because (at the end) CSS_PATH is going to be passed from the main window to this plugin, so please remove them.
@@ -534,7 +539,7 @@ class IPythonConsole(SpyderPluginWidget): | |||
"make it writable.") | |||
|
|||
def __init__(self, parent, testing=False, test_dir=None, | |||
test_no_stderr=False): | |||
test_no_stderr=False, css_path=CSS_PATH): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change this to css_path=None
@@ -110,7 +110,8 @@ def __init__(self, plugin, id_, | |||
options_button=None, | |||
show_elapsed_time=False, | |||
reset_warning=True, | |||
ask_before_restart=True): | |||
ask_before_restart=True, | |||
css_path=CSS_PATH): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change this to css_path=None
.
@@ -135,6 +136,7 @@ def __init__(self, plugin, id_, | |||
self.allow_rename = True | |||
self.stderr_dir = None | |||
self.is_error_shown = False | |||
self.css_path = css_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change this to
if css_path is None:
self.css_path = CSS_PATH
else:
self.css_path = css_path
This way tests for the IPython console alone will pass without problems.
Great call; it looks amazing!
We can always come back to it later and get e.g. @jnsebgosselin to weigh in. However, I'm not sure where in the Darkly theme it is intended to have warning dialogs look like that; the closest I see is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @dalthviz!! Really great job here!
The css is realy great !! But it doesn't work for table ! As an example try to get the documentation about scipy.optimize. The background is white and with a white text it becomes unreadable. Thanks for your work |
Thanks @Ticonderoga ! Here's an example screenshot of what you're describing; also, it shows the scrollbar is still unthemed. I'll add it to #8068 . |
@dalthviz, please solve this problem. |
Pull Request Checklist
Developer Certificate of Origin Affirmation
By submitting this Pull Request or typing my name below, I affirm the
I certify the above statement is true and correct: dalthvizDeveloper Certificate of Origin
with respect to both the content of the contribution itself and this post,
and understand I am releasing it under Spyder's MIT (Expat) license.
Description of Changes
Add CSS for the Help plugin in a dark theme.
Issue(s) Resolved
Part of #8068
Fixes #8087