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
Load theme as extension (continued) #2283
Conversation
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.
Just a few minor comments. Will wait for reply but probably ready for merging.
@@ -8,7 +8,7 @@ | |||
"dependencies": { | |||
"@jupyterlab/codemirror": "^0.5.0", | |||
"@jupyterlab/console": "^0.5.1", | |||
"@jupyterlab/default-theme": "^0.5.1", | |||
"@jupyterlab/theming": "^0.5.1", |
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.
In my PR, I entirely removed the theming assets from the individual packages. The idea was to not hard code those assets into other packages but let the theme itself choose to include them or not. That would allow themes to include the default assets, or provide their own set of them.
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.
Opps missed that this is an example, ignore that comment. Examples definitely need to include the themeing.
@@ -36,19 +31,24 @@ body { | |||
} | |||
|
|||
|
|||
.p-Widget.p-mod-hidden { | |||
display: none !important; | |||
#jp-main-dock-panel { |
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.
Did we mean for this stuff to be here?
@@ -38,8 +38,8 @@ | |||
} | |||
|
|||
|
|||
.jp-SideBar .p-TabBar-content, | |||
.jp-SideBar .p-TabBar-content { | |||
.jp-SideBar.p-TabBar .p-TabBar-content, |
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.
Why more specificity needed?
I am going to do a bunch of work on design stuff now, going to merge this and we can iterate more in further PRs if needed. |
Split out
default-theme
intotheming
andtheme-light-extension
and load it as an extension. Fixes #2165 and replaces #2169 and finishes #2205.