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

fix(DockLayout): Invalid use of "this" in private namespace #281

Merged
merged 1 commit into from
Jan 13, 2022

Conversation

GordonSmith
Copy link
Contributor

Signed-off-by: Gordon Smith GordonJSmith@gmail.com

@@ -2193,7 +2193,7 @@ namespace Private {
renderer: DockLayout.IRenderer
): TabLayoutNode {
// Create the tab bar for the layout node.
let tabBar = renderer.createTabBar(this._document);
let tabBar = renderer.createTabBar();
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to be able to pass an optional document to realizeTabAreaConfig to be forward to createTabBar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe - I am struggling to see where its used (as it doesn't seem to be hit in the tests and the old version of TypeScript isn't helpful).
I will have more time to dig into it in the morning, if your not looking for a quick fix today,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fcollonval: I changed the code as you suggested and did a bit more sanity checking...

Also I added TabPanel support to the web-components: https://gordonsmith.github.io/hpcc-js-wc/src/layouts/lumino/tabPanel.html

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks a lot for finalizing this one.

We should not change the signature of createTabBar as its part of the public API. Otherwise this is looking good.

@@ -1190,7 +1194,7 @@ export namespace DockLayout {
*
* @returns A new tab bar for a dock layout.
*/
createTabBar(document?: Document | ShadowRoot): TabBar<Widget>;
createTabBar(document: Document | ShadowRoot): TabBar<Widget>;
Copy link
Member

Choose a reason for hiding this comment

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

We should keep it optional as this is part of the public API.

Suggested change
createTabBar(document: Document | ShadowRoot): TabBar<Widget>;
createTabBar(document?: Document | ShadowRoot): TabBar<Widget>;

@@ -1400,7 +1400,7 @@ export namespace DockPanel {
*
* @returns A new tab bar for a dock panel.
*/
createTabBar(document?: Document | ShadowRoot): TabBar<Widget> {
createTabBar(document: Document | ShadowRoot): TabBar<Widget> {
Copy link
Member

Choose a reason for hiding this comment

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

We should keep it optional as this is part of the public API.

Suggested change
createTabBar(document: Document | ShadowRoot): TabBar<Widget> {
createTabBar(document?: Document | ShadowRoot): TabBar<Widget> {

Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

LGTM thanks @GordonSmith

@blink1073
Copy link
Member

Thanks! I'll cut a bugfix release now.

@blink1073 blink1073 merged commit f7d36a8 into jupyterlab:main Jan 13, 2022
Comment on lines +349 to +350
createTabBar: () => this._createTabBar(),
createHandle: () => this._createHandle()
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry @GordonSmith I have a doubt due to this intermediate renderer definition. Actually, the this._document pass here is not use as these two callbacks are not passing through the received argument (and it does not make sense to allow a document other than this._document to be passed).
And as this is the only place Private.realizeAreaConfig is called, it sounds that your initial commit only removing this._document in Private.realizeTabAreaConfig is the only needed change, isn't it?

Copy link
Contributor Author

@GordonSmith GordonSmith Jan 13, 2022

Choose a reason for hiding this comment

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

I think its ok as they are just there to satisfy the IRenderer and resolve back onto itself (which is your point):

  private _createTabBar(): TabBar<Widget> {
    // Create the tab bar using the renderer.
    let tabBar = this.renderer.createTabBar(this._document);

...
  }

IOW changing to this:

          createTabBar: (document?: Document | SahdowRoot) => this._createTabBar(this._document),
          createHandle: () => this._createHandle()

Ends up exactly the same (again this is the point your making?)

You could argue that the following is better from a self-documenting point of view?

          createTabBar: (_document?: Document | SahdowRoot) => this._createTabBar(),
          createHandle: () => this._createHandle()

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detailed answer. I agree with your latest proposal:

          createTabBar: (document?: Document | ShadowRoot) => this._createTabBar(),
          createHandle: () => this._createHandle()

It will indeed be a better self-documenting code. I opened #285 to add this.

@hbcarlos
Copy link
Member

Thanks for the fix @GordonSmith!

@GordonSmith GordonSmith deleted the REGRESION branch January 14, 2022 16:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants