Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Fix icons for the dark theme #376

Merged
merged 7 commits into from
Mar 14, 2020
Merged

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Mar 2, 2020

Fixes #375.

image

TODO

  • Callstack icons
  • Debugger icon in the toolbar

@jtpio jtpio changed the title Fix callstack icons for the dark theme Fix icons for the dark theme Mar 2, 2020
@jtpio
Copy link
Member Author

jtpio commented Mar 2, 2020

Now sounds like a good time to migrate to the new LabIcon.

<svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg">
<path fill-rule="evenodd" clip-rule="evenodd" d="M7.99998 9.53198H8.54198L12.447 5.62698L11.386 4.56698L8.74898 7.17698L8.74898 0.999985H7.99998H7.25098L7.25098 7.17698L4.61398 4.56698L3.55298 5.62698L7.45798 9.53198H7.99998ZM9.95598 13.013C9.95598 14.1175 9.06055 15.013 7.95598 15.013C6.85141 15.013 5.95598 14.1175 5.95598 13.013C5.95598 11.9084 6.85141 11.013 7.95598 11.013C9.06055 11.013 9.95598 11.9084 9.95598 13.013Z" fill="#616161"/>
<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg">
<g class="jp-icon3" fill="#616161">
Copy link
Member Author

Choose a reason for hiding this comment

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

The jp-icon3 is needed for the icons to be styled correctly based on the theme (using the correct fill color).

@jtpio
Copy link
Member Author

jtpio commented Mar 2, 2020

31d8bb0 should fix most of the icons.

Still the debugger toolbar icon to fix.

@jtpio
Copy link
Member Author

jtpio commented Mar 2, 2020

949b280 adds the debugger icon and the button as two separate items in the toolbar.

As a side effect the hover style now applies only to the switch:

image

cc @telamonian do you know if there is a more canonical way of achieving this with the new LabIcon system and the bugIcon from ui-components?

Until now the toolbar component was defined with:

debugger/src/handler.ts

Lines 38 to 46 in 2ee7a2e

const button = new ToolbarButton({
className: 'jp-DebuggerSwitchButton',
iconClass: 'jp-ToggleSwitch',
onClick,
tooltip: 'Enable / Disable Debugger'
});
widget.toolbar.addItem('debugger-button', button);
return button;

And styled with:

debugger/style/icons.css

Lines 90 to 122 in 2ee7a2e

.jp-DebuggerSwitchButton::before {
content: '';
width: 14px;
height: 14px;
background-position: left center;
background-repeat: no-repeat;
background-size: 14px;
margin-right: 4px;
background-image: var(--jp-icon-bug);
}
.jp-Toolbar-item .jp-ToggleSwitch {
display: flex;
align-items: center;
cursor: pointer;
background-color: var(--jp-border-color1);
-webkit-transition: 0.4s;
transition: 0.4s;
border-radius: 34px;
width: 35px;
}
.jp-Toolbar-item .jp-ToggleSwitch::before {
content: '';
height: 10px;
width: 10px;
margin-left: 5px;
margin-right: 50%;
background-color: white;
-webkit-transition: 0.4s;
transition: 0.4s;
border-radius: 50%;
}

Which made it possible to have both jp-BugIcon and the custom switch, visually looking like the following:

image

@jtpio jtpio marked this pull request as ready for review March 2, 2020 15:32
@jtpio
Copy link
Member Author

jtpio commented Mar 3, 2020

Let's make the bug icon clickable as well is we keep the two items separate.

@jtpio
Copy link
Member Author

jtpio commented Mar 3, 2020

d7e58e4 implements this:

bug-icon-switch

It kind of feels "odd" that two items of the toolbar do the same thing though.

@jtpio
Copy link
Member Author

jtpio commented Mar 4, 2020

Maybe @tgeorgeux has some input for a nice switch button UX that includes an icon? It looks like core JupyterLab doesn't have such component yet, so it might still be uncertain what we want to have really.

Otherwise I suggest handling this as a separate issue.

@telamonian
Copy link
Member

cc @telamonian do you know if there is a more canonical way of achieving this with the new LabIcon system and the bugIcon from ui-components?

Short answer is no. I am currently thinking about ways to support multiple icons/text labels in a single LabButton (or something) component. Part of that is figuring out the actual use cases, so thanks for this.

It's a bit hacky, but I think you can get what you really want (a single outer <button> with two child icons side by side) like this:

const button = new ToolbarButton({ 
   className: classes('jp-DebuggerSwitchButton', 'jp-ToggleSwitch'),
   icon: bugIcon, 
   onClick, 
   tooltip: 'Enable / Disable Debugger' 
 }); 

So the bug icon would be inline svg, while the toggle switch would continue to be CSS. Getting the styling right might be a bit tricky, though

@jtpio
Copy link
Member Author

jtpio commented Mar 4, 2020

Thanks @telamonian for the input!

I was also thinking about building a custom React component directly to have full control on it. Maybe that would be the simpler solution for now.

const button = new ToolbarButton({ 
   className: classes('jp-DebuggerSwitchButton', 'jp-ToggleSwitch'),
   icon: bugIcon, 
   onClick, 
   tooltip: 'Enable / Disable Debugger' 
 }); 

So the bug icon would be inline svg, while the toggle switch would continue to be CSS. Getting the styling right might be a bit tricky, though.

Yes, this is basically how it is at the moment (except that is uses iconClass instead of icon).

@telamonian
Copy link
Member

Yes, this is basically how it is at the moment (except that is uses iconClass instead of icon).

The difference would be that both bug icon and toggle icon would be children of the same large button, like before, instead of having

two items of the toolbar do the same thing though.

@jtpio
Copy link
Member Author

jtpio commented Mar 6, 2020

@afshin It looks like something like this works well actually:

bug-icon-switch-label

@afshin
Copy link
Member

afshin commented Mar 6, 2020 via email

@tgeorgeux
Copy link
Contributor

@jtpio I am working on switches here: jupyterlab/jupyterlab#8010

I think if the icon is being used as a toggle it makes more sense to color the icon to indicate it's on or off. The design pattern doesn't exist in Jupyterlab to have an 'icon and toggle' together. I

Hope this helps, I'll be in touch.

@jtpio
Copy link
Member Author

jtpio commented Mar 9, 2020

Thanks @tgeorgeux for opening this issue 👍

@jtpio jtpio requested a review from afshin March 10, 2020 15:16
Copy link
Member

@afshin afshin left a comment

Choose a reason for hiding this comment

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

Thank you!

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Icons with the dark theme
4 participants