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

Split monolithic datalab.js into smaller files #1318

Merged
merged 6 commits into from Apr 17, 2017
Merged

Conversation

jimmc
Copy link
Contributor

@jimmc jimmc commented Apr 17, 2017

The code that has been pulled out of datalab.js into other files has not been changed in any way other than whitespace changes.

@jimmc jimmc requested a review from yebrahim April 17, 2017 19:56
Copy link
Contributor

@yebrahim yebrahim left a comment

Choose a reason for hiding this comment

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

Just some minor changes, otherwise LGTM.

window.dataLayer.push(event);
}

function toggleSidebar() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think toggleSidebar, showHelp (and the following markup snippet), and lines 197-227 should move into notebook-app.js.

Should this file be called app-bar.js? It consists mostly of the common app bar scripts, like the About, restarting and managing VM, sign in/out.. etc. Common code like the xhr function should perhaps stay in datalab.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed page.js to appbar.js and moved some common functions back into datalab.js.

As discussed offline, I will leave toggleSidebar and showHelp here for now, until we can figure out a cleaner detangling of appbar and notebook-app.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. I'm actually surprised this seems to be the only place we got things tangled up, given we've been editing this massive file for a long time now.

@@ -0,0 +1,93 @@
function shouldShimWebsockets() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use two space tabs for this file for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jimmc jimmc merged commit 8a86bf5 into master Apr 17, 2017
@jimmc jimmc deleted the jimmc-client-refactor branch April 17, 2017 23:35
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.

None yet

2 participants