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

Redesign of sidebars and buttons #5117

Merged
merged 35 commits into from Aug 15, 2018
Merged

Conversation

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Aug 13, 2018

This moves the L/R sidebars to be icon based and fixes some issues with our buttons. Still working on the last bits, but targeting for 0.34.

@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Aug 13, 2018

Here is the direction:

screen shot 2018-08-12 at 9 15 51 pm

@tgeorgeux

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Aug 13, 2018

Cool stuff. This, together with L/R docking will go a long way, I think.

I get the metaphor of the running icon, but to me it also looks a lot like the exit sign, which is a touch confusing. I wonder if something like the run button used on the notebook toolbar, or the kernel icon developed for the status bar, might be clearer.

@vidartf
Copy link
Member

@vidartf vidartf commented Aug 13, 2018

Does this change require that extensions supply its own icon, or is this change for core extensions only? In general, I'm missing a mechanism for extensions to supply icons that work well with themes (although the PR with isLight for the theme helps quite a bit).

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Aug 13, 2018

This looks good. Did you play around at all with the idea of a smaller 'tab' portion when selected? I'm curious if it would help the selected tab stand out a little bit more. Either way is good with me, looks a lot better than what we had!

@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Aug 13, 2018

@vidartf extensions can still use text and it will render fine. If they want to provide an icon:

  1. It has to be in our themes, or
  2. it has to be shipped with the extension and use the new isLight logic to style itself. That is one reason we were holding off on this one.

@ian-r-rose yeah, we sat with the designers for hours on Friday hunting for good icons to use. I agree with your points, but probably still prefer to running person by a slight margin: The "play" button doesn't quite capture that this panel lets you view things that are running but doesn't allow you to run them. The kernel icon doesn't capture that it lists terminals as well. Are you ok with us "running" with this one for now and see how it goes?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Aug 13, 2018

Yep, no objection to moving forwards with this for now. I do see the possibility of people confusing it with a exiting/quitting-type action, but we shall see!

@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Aug 14, 2018

OK a short update in light of the upcoming release. The current state of PR now has refactored toolbar buttons that are ready for the final styling. Will keep pushing hard on it...

@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Aug 14, 2018

Making lots of progress, final todo list:

  • Refactor kernel name button in apputils/toobar.tsx
  • Finish styling running sidebar
  • Fix buttons in advanced settings editor
  • Fix styling of dialog buttons
  • Make sure tests are ok
  • Changelog entry

@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Aug 15, 2018

@blink1073 Here is the status: everything is working and all styles are updated. However, my brain is mush so I haven't finished fixing the tests. Not sure exactly what is worth doing as the original tests made strong assumptions about the ToolbarButton rendering itself in a particular way, with particular children and all as a phosphor Widget + native DOM nodes. In the new approach the Widget is a very thin wrapper around a react component - so none of the properties, event or methods that were there make sense any more. Some of the tests could probably be fixed easily by someone more awake that me, but it also may make sense to delete some of them. This PR is such a huge improvement, I do think it should be merged for 0.34. I will be around briefly in the morning 8-9am PT and then later in the day.

@ellisonbg ellisonbg changed the title [WIP] Redesign of sidebars and buttons Redesign of sidebars and buttons Aug 15, 2018
@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Aug 15, 2018

OK I am back and will work on the tests now...

@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Aug 15, 2018

OK I think the test are passing. I am running them locally and just pushed to let CI do its thing too.

@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Aug 15, 2018

OK tests should be fixed now. Rerunning locally and on CI.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 15, 2018

Very nice, thanks!

@blink1073 blink1073 merged commit a2c5f68 into jupyterlab:master Aug 15, 2018
1 of 2 checks passed
@nanoant
Copy link
Contributor

@nanoant nanoant commented Aug 24, 2018

@ellisonbg Just wanted to ask if menu bar is still white in the latest iteration of the design. FYI I reported some issue #4148 about that. If not I am attaching this PR to my issue, as I believe menu should be gray -- which is btw. less intrusive, like rest of the tab bars, and only actively selected item in the menu should be white as it was in the original #16 design.

@stonebig
Copy link

@stonebig stonebig commented Aug 24, 2018

for Jupyterlab-0.34.2 ?

@@ -70,7 +70,7 @@ export namespace ToolbarItems {
*/
export function createSaveButton(panel: NotebookPanel): ToolbarButton {
return new ToolbarButton({
className: TOOLBAR_SAVE_CLASS,
iconClassName: TOOLBAR_SAVE_CLASS + ' foo jp-Icon jp-Icon-16',
Copy link
Contributor

@jasongrout jasongrout Dec 29, 2018

Choose a reason for hiding this comment

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

Oops, looks like we accidentally added the class foo.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants