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

Add a clone method to ABCWidgetFactory. #6060

Merged
merged 1 commit into from May 23, 2019

Conversation

@Madhu94
Copy link
Collaborator

@Madhu94 Madhu94 commented Mar 3, 2019

Fixes #6044

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Mar 3, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@afshin afshin added this to the 1.0 milestone Mar 27, 2019
@afshin afshin removed this from the 1.0 milestone Mar 27, 2019
@afshin afshin added this to the 1.1 milestone Mar 27, 2019
Copy link
Member

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

Thanks @Madhu94, and sorry for the slow response time! This looks good to me, can you add a couple of very basic tests to the test suite so that this new function is exercised (I'm thinking one in docregistry and one in docmanager).

Also pinging @jasongrout for feedback.

packages/docregistry/src/registry.ts Outdated Show resolved Hide resolved
packages/docmanager/src/widgetmanager.ts Outdated Show resolved Hide resolved
/**
* Clone a new widget given a context
*/
clone(context: IContext<U>): T;
Copy link
Contributor

@jasongrout jasongrout Apr 20, 2019

In the particular case I was thinking of, where two clones of a notebook widget should share the same rendermime, the clone method needs the widget it is cloning as well, not just the context. Can we add an argument here for the widget of type T that we are cloning?

Copy link
Collaborator Author

@Madhu94 Madhu94 May 5, 2019

I had to do some typecasting for docManager.cloneWidget to achieve this- I'd really like some feedback on whether that's the right approach.

Copy link
Contributor

@jasongrout jasongrout May 20, 2019

The changes look great! I think we ought to just make cloneWidget only take in IDocumentWidgets instead of typecasting them. If you can make that change, I'm good with merging this. Thanks again!

@Madhu94 Madhu94 force-pushed the abcwidgetfactory-clone-method branch 2 times, most recently from dcf02f7 to d3de616 Apr 28, 2019
@Madhu94 Madhu94 force-pushed the abcwidgetfactory-clone-method branch from d3de616 to ac81b73 May 5, 2019
@Madhu94 Madhu94 force-pushed the abcwidgetfactory-clone-method branch from 51f3f29 to 7427823 May 5, 2019
@jasongrout jasongrout removed this from the 1.1 milestone May 20, 2019
@jasongrout jasongrout added this to the 1.0 milestone May 20, 2019
@jasongrout jasongrout force-pushed the abcwidgetfactory-clone-method branch 2 times, most recently from d1e490b to 7427823 May 23, 2019
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 23, 2019

My changes are a bit of a rabbit hole and are trickier than I had hoped. So let's merge this for now, and I'll push my changes to another PR.

Thanks again!

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented May 30, 2019

Thanks @jasongrout and apologies for the delay, I couldn't (can't) devote more time to lab at this point in time.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 30, 2019

Thank you for the contributions you have made @Madhu94!

@lock
Copy link

@lock lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 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.

4 participants