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

Main area widget upgrades #3499

Merged
merged 20 commits into from Apr 27, 2018
Merged

Main area widget upgrades #3499

merged 20 commits into from Apr 27, 2018

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 30, 2017

Partially addresses #3495.

  • Updates the MainAreaWidget to include a Toolbar and to handle title and focus.
  • Uses the MainAreaWidget in all of the top level widgets except for the Console and the document widgets, which will come in subsequent PRs

image

@blink1073 blink1073 added this to the 1.0 milestone Dec 30, 2017
@blink1073 blink1073 changed the title [WIP] Main area widget upgrades Main area widget upgrades Jan 3, 2018
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jan 3, 2018

This is marked for 1.0 but is ready.

@blink1073 blink1073 self-assigned this Jan 3, 2018
case 'mouseout':
let target = event.target as HTMLElement;
if (this.toolbar.node.contains(document.activeElement) &&
target.localName !== 'select') {
Copy link
Member

@afshin afshin Jan 4, 2018

I think this expression should be:

target.tagName !== 'SELECT'

The localName attribute is deprecated: https://developer.mozilla.org/en-US/docs/Web/API/Node/localName

@@ -4,6 +4,6 @@
| Distributed under the terms of the Modified BSD License.
|----------------------------------------------------------------------------*/

.jp-MainAreaWidget:focus {
.jp-MainAreaWidget > *:focus {
Copy link
Member

@afshin afshin Jan 4, 2018

If the intention is any immediate descendant of .jp-MainAreaWidget that has focus, then the * is unnecessary and .jp-MainAreaWidget > :focus is sufficient.

@blink1073 blink1073 force-pushed the main-area-widget branch 3 times, most recently from 0a5bc4a to 8f5a9aa Jan 5, 2018
afshin
afshin approved these changes Jan 5, 2018
Copy link
Member

@afshin afshin left a comment

This is great, thanks! I tried it out locally on Chrome and Firefox. Works as advertised and is a nice abstraction.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jan 7, 2018

I am torn on this - it is a big change to land right before the beta and I haven't had the chance to review the API changes. How much code review has this gotten?

@blink1073 blink1073 removed this from the Beta 1 milestone Jan 7, 2018
@blink1073 blink1073 added this to the Beta 2 milestone Jan 7, 2018
@afshin
Copy link
Member

@afshin afshin commented Jan 7, 2018

@ellisonbg I built it and tested it out, but it is a big change. I think the decision to move it to https://github.com/jupyterlab/jupyterlab/milestone/11 is good.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jan 7, 2018

+1 on targeting this to Beta 2

*
* #### Notes
* This method implements the DOM `EventListener` interface and is
* called in response to events on the dock panel's node. It should
Copy link
Member

@afshin afshin Jan 15, 2018

dock panel => main area widget

@blink1073 blink1073 mentioned this pull request Feb 5, 2018
@blink1073 blink1073 force-pushed the main-area-widget branch 2 times, most recently from d3c1d71 to be92105 Feb 6, 2018
blink1073 added 4 commits Feb 15, 2018
wip main area widget updates

Update help extension

Use main area widget where applicable

clean up main area widget handling

finish main area widgets

move activation to docregistry options

Update main area widget tests

update tests and integrity

Update tests

integrity update

address review

fix conflicts

Remove coverage files

Remove coverage files
@blink1073 blink1073 force-pushed the main-area-widget branch from be92105 to 8d3baa6 Feb 15, 2018
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 21, 2018

Ping @blink1073 - should we block on this for beta 2?

@jasongrout jasongrout removed this from the Beta 2 milestone Mar 28, 2018
@jasongrout jasongrout added this to the Beta 3 milestone Mar 28, 2018
@jasongrout jasongrout mentioned this pull request Apr 26, 2018
18 tasks
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 27, 2018

@afshin, @ian-r-rose, I think this is good to go in now. We'll iterate on this more in #4453 too.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 27, 2018

Can one of you look over my changes?

@jasongrout jasongrout force-pushed the main-area-widget branch from 3bb4ab4 to ee0e555 Apr 27, 2018
Copy link
Member

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

This looks great, and removes a lot of duplicated logic! A few minor comments, and then I am happy.

if (manager.inspector.isAttached) {
shell.activateById(manager.inspector.id);
if (!manager.inspector.isAttached) {
shell.addToMainArea(manager.inspector.parent, { activate: false });
Copy link
Member

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

Going through the parent doesn't seem particularly clean here. Why not have newInspectorPanel return the MainAreaWidget that is created, and operate with that in the execute() function?

Copy link
Contributor

@jasongrout jasongrout Apr 27, 2018

Sure - would you be able to take that up in a new PR? I didn't concentrate a lot on this bit of the PR.

Copy link
Member

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

Sure.

Copy link
Contributor

@jasongrout jasongrout Jul 26, 2018

@ian-r-rose - did you make an issue for your comment?

Copy link
Member

@ian-r-rose ian-r-rose Jul 26, 2018

If memory serves, I made a branch to fix this, and found that it would be rather more work to refactor it than I had planned, so I dropped it.

@@ -34,18 +34,6 @@
}


.jp-Launcher::before {
Copy link
Member

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

I am very happy to see these go!

height: calc(100% - var(--jp-toolbar-micro-height));
}

.jp-LinkedOutputView-toolbar {
Copy link
Member

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

This box-shadow styling that the notebook has and the linked output used to have are kind of nice. Maybe we should bring them over to the MainAreaWidget?

Copy link
Contributor

@jasongrout jasongrout Apr 27, 2018

Good idea.

Copy link
Contributor

@jasongrout jasongrout Apr 27, 2018

I'll submit a follow-up PR that can finish testing while I move on to the document widget...

@@ -28,20 +27,9 @@
}


#setting-editor::before {
Copy link
Member

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

👍

afshin
afshin approved these changes Apr 27, 2018
Copy link
Member

@afshin afshin left a comment

Awesome, thanks @jasongrout, for picking this up!

@jasongrout jasongrout merged commit 64183d8 into jupyterlab:master Apr 27, 2018
2 checks passed
@blink1073 blink1073 deleted the main-area-widget branch Jul 16, 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

5 participants