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

Bottom Panel Implementation #4752

Merged
merged 11 commits into from Jun 27, 2018
Merged

Bottom Panel Implementation #4752

merged 11 commits into from Jun 27, 2018

Conversation

@richagadgil
Copy link
Contributor

@richagadgil richagadgil commented Jun 20, 2018

Implemented a bottom panel to go alongside the top, main, and side panels. Can be populated with widgets. Meant to be groundwork for status bar, etc.
@ellisonbg

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 20, 2018

This looks great to me. My only concern is with the height of the bottom panel: it is zero when there is nothing in it (good!), but when something is added to it it remains zero (bad!). Am I missing something? @jasongrout do you think this would be a good candidate for your ToolbarLayout?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2018

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2018

Wait. The status bar added to the bottom panel should have the toolbar layout logic. The bottom panel itself should not.

@richagadgil
Copy link
Contributor Author

@richagadgil richagadgil commented Jun 25, 2018

I just modified the code so that the bottom panel keeps its minimum height, but stays hidden until widgets are added to it.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 25, 2018

@jasongrout I'm not sure what you mean when you say the bottom panel should not have toolbar layout logic: why not?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 25, 2018

@jasongrout I'm not sure what you mean when you say the bottom panel should not have toolbar layout logic: why not?

I think for now, the bottom panel should have exactly the same logic as the top panel. Glancing at the code, it looks like it does, so I think we're good!

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 25, 2018

Well, it's not exactly the same: the top area has a hard-coded height, whereas the bottom area dynamically hides/shows depending on whether there is anything in it. I think that's the right behavior, but I wonder whether we should use this implementation or the ToolbarLayout for that.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 25, 2018

@jasongrout I'm not sure what you mean when you say the bottom panel should not have toolbar layout logic: why not?

The thought process was that if the status bar doesn't have any items in it, then it should auto-collapse its height, and the bottom panel would just follow along, i.e., the bottom panel's min-height would be the min-height of the status bar. However, it's a moot point if the status bar is always displaying no matter if there are widgets or not.

For now, I think it's fine to just have the same logic as the top bar.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 25, 2018

Well, it's not exactly the same: the top area has a hard-coded height, whereas the bottom area dynamically hides/shows depending on whether there is anything in it.

Do you mean the css min-height is forcing the top and bottom panel to show, even if there is nothing in them, right now?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 25, 2018

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 25, 2018

(Update: in an in-person video conference, we decided it was better to switch both the top and bottom panels to use a box layout rather than just a panel layout).

@jasongrout jasongrout added this to the Beta 3 milestone Jun 27, 2018
@@ -96,6 +99,7 @@ class ApplicationShell extends Widget {
rightHandler.sideBar.addClass('jp-mod-right');
rightHandler.stackedPanel.id = 'jp-right-stack';

bottomPanel.direction = 'left-to-right';
Copy link
Member

@ian-r-rose ian-r-rose Jun 27, 2018

Why change the layout direction to left-to-right? I had envisioned people possibly stacking things on top of the status bar.

@richagadgil
Copy link
Contributor Author

@richagadgil richagadgil commented Jun 27, 2018

@ian-r-rose That makes sense for the outermost components -- changed the direction in the latest commit to 'bottom-to-top.'

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 27, 2018

Great, that makes sense to me.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Looks great, thanks @richagadgil !

@ian-r-rose ian-r-rose merged commit 2e10c68 into jupyterlab:master Jun 27, 2018
2 checks passed
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@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

3 participants