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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jan 13, 2022
+18 −11
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions packages/widgets/src/docklayout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,10 +343,14 @@ export class DockLayout extends Layout {

// Create the root node for the new config.
if (mainConfig) {
this._root = Private.realizeAreaConfig(mainConfig, {
createTabBar: () => this._createTabBar(),
createHandle: () => this._createHandle()
});
this._root = Private.realizeAreaConfig(
mainConfig,
{
createTabBar: () => this._createTabBar(),
createHandle: () => this._createHandle()
Comment on lines +349 to +350
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 •
edited
Loading

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.

},
this._document
);
} else {
this._root = null;
}
Expand Down Expand Up @@ -1482,13 +1486,14 @@ namespace Private {
*/
export function realizeAreaConfig(
config: DockLayout.AreaConfig,
renderer: DockLayout.IRenderer
renderer: DockLayout.IRenderer,
document: Document | ShadowRoot
): LayoutNode {
let node: LayoutNode;
if (config.type === 'tab-area') {
node = realizeTabAreaConfig(config, renderer);
node = realizeTabAreaConfig(config, renderer, document);
} else {
node = realizeSplitAreaConfig(config, renderer);
node = realizeSplitAreaConfig(config, renderer, document);
}
return node;
}
Expand Down Expand Up @@ -2190,10 +2195,11 @@ namespace Private {
*/
function realizeTabAreaConfig(
config: DockLayout.ITabAreaConfig,
renderer: DockLayout.IRenderer
renderer: DockLayout.IRenderer,
document: Document | ShadowRoot
): TabLayoutNode {
// Create the tab bar for the layout node.
let tabBar = renderer.createTabBar(this._document);
let tabBar = renderer.createTabBar(document);

// Hide each widget and add it to the tab bar.
each(config.widgets, widget => {
Expand All @@ -2214,15 +2220,16 @@ namespace Private {
*/
function realizeSplitAreaConfig(
config: DockLayout.ISplitAreaConfig,
renderer: DockLayout.IRenderer
renderer: DockLayout.IRenderer,
document: Document | ShadowRoot
): SplitLayoutNode {
// Create the split layout node.
let node = new SplitLayoutNode(config.orientation);

// Add each child to the layout node.
each(config.children, (child, i) => {
// Create the child data for the layout node.
let childNode = realizeAreaConfig(child, renderer);
let childNode = realizeAreaConfig(child, renderer, document);
let sizer = createSizer(config.sizes[i]);
let handle = renderer.createHandle();

Expand Down