split platform-dock-layout

[Sebastian-ubs]

This change is 

[Sebastian-ubs]

@tjcouch-sil could you please have a look at this?

[Sebastian-ubs]

Hi @tjcouch-sil could you have a look why this says

A spread argument must either have a tuple type or be passed to a rest parameter.ts(2556)

[Sebastian-ubs]

@tjcouch-sil: Is this correct? At least it renders :-)

[Sebastian-ubs]

If this approach looks good, I will also take care for the test code and thinking of moving / renaming the scss file.

[tjcouch-sil]

Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions (waiting on @Sebastian-ubs)

---

src/renderer/components/docking/platform-dock-layout-wrapper.component.tsx line 20 at r1 (raw file):

const DockLayoutWrapper = forwardRef(function DockLayoutWrapper(
  { loadTab, saveTab, onLayoutChange, defaultLayout, style }: DockLayoutWrapperProps,
  ref: LegacyRef<DockLayout> | undefined,

Though I am not familiar with it, I imagine LegacyRef likely shouldn't be used anymore. Have you tried ForwardedRef?

Here's the TypeScript React cheetsheet page on forwardRef for reference: https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/forward_and_create_ref/

[tjcouch-sil]

Thanks for doing this! Yeah, the forwardRef idea you implemented here is great.

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Sebastian-ubs)

---

src/renderer/components/docking/platform-dock-layout-sizing.tsx line 1 at r1 (raw file):

import {

These new utility files need .util.ts at the end instead of .tsx: as mentioned in the style guide "Filenames and paths" section, we need a type on the filename. Additionally, .tsx is used for React component files. These new files should hopefully be able to be changed into .ts files unless I'm missing something.

---

src/renderer/components/docking/platform-dock-layout-sizing.tsx line 10 at r1 (raw file):

import { FloatPosition, FloatSize, LayoutSize, TabGroup } from 'rc-dock';
import cloneDeep from 'lodash/cloneDeep';
import DIALOGS from '../dialogs';

Here and in many places in this PR, please replace these relative paths with path aliases as mentioned in the "Prefer path aliases for imports" line of the style guide.

For example, here, this import would instead be the following:

import DIALOGS from '@renderer/components/dialogs';

---

src/renderer/components/docking/platform-dock-layout-wrapper.component.tsx line 9 at r1 (raw file):

  loadTab: (savedTabInfo: SavedTabInfo) => RCDockTabInfo;
  saveTab: (dockTabInfo: RCDockTabInfo) => SavedTabInfo | undefined;
  onLayoutChange: (

It may be best to use the OnLayoutChangeRCDock type here for consistency.

---

src/renderer/components/docking/platform-dock-layout.component.tsx line 80 at r1 (raw file):

}

function onLayoutChange(

This function's name is misleading; it does not indicate that it is returning a function but appears to be doing some execution. I recommend calling it createOnLayoutChangeCallback or something or preferably changing it back to being in-line since it is not used in multiple places, will have types implicitly, and will not require a name. Also, if you keep it split out, please move it above the function component according to the style guide.

---

src/renderer/components/docking/platform-dock-layout.component.tsx line 85 at r1 (raw file):

) {
  /* eslint-disable-next-line */
  return (...args: any[]) => {

You may be seeing the weird errors here because this is not typed. One disadvantage of moving this away from being in-line is that it does not come along with types. The correct types here would be Parameters<OnLayoutChangeRCDock>. And the return type on this function should be ReturnType<OnLayoutChangeRCDock>.

Please remove the eslint-disable-next-line as it is too generic. Hopefully we can solve the problem that led to adding that. If we can't solve the types issues, please use Intellisense to figure out the specific eslint rule to disable and explain the exception in a comment above it as specified in "Explain rule exceptions" in the style guide

---

src/shared/models/docking-framework.model.ts line 197 at r1 (raw file):

export type TabType = string;

export type RCDockTabInfo = TabData & TabInfo;

I would much prefer this type to stay internal to the docking framework files to avoid leaking out the rc-dock implementation details into more files than it already is in. Could we move these three additions (TabType, RCDockTabInfo, and isTab) back into the component file or into a single platform-dock-layout.util.ts file that encompasses all the new utility files to avoid having to export RCDockTabInfo and TabType?

[Sebastian-ubs]

Reviewable status: 0 of 14 files reviewed, 9 unresolved discussions (waiting on @tjcouch-sil)

---

src/renderer/components/docking/platform-dock-layout.component.tsx line 80 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This function's name is misleading; it does not indicate that it is returning a function but appears to be doing some execution. I recommend calling it createOnLayoutChangeCallback or something or preferably changing it back to being in-line since it is not used in multiple places, will have types implicitly, and will not require a name. Also, if you keep it split out, please move it above the function component according to the style guide.

done

---

src/renderer/components/docking/platform-dock-layout.component.tsx line 85 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

You may be seeing the weird errors here because this is not typed. One disadvantage of moving this away from being in-line is that it does not come along with types. The correct types here would be Parameters<OnLayoutChangeRCDock>. And the return type on this function should be ReturnType<OnLayoutChangeRCDock>.

Please remove the eslint-disable-next-line as it is too generic. Hopefully we can solve the problem that led to adding that. If we can't solve the types issues, please use Intellisense to figure out the specific eslint rule to disable and explain the exception in a comment above it as specified in "Explain rule exceptions" in the style guide

done

---

src/shared/models/docking-framework.model.ts line 197 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I would much prefer this type to stay internal to the docking framework files to avoid leaking out the rc-dock implementation details into more files than it already is in. Could we move these three additions (TabType, RCDockTabInfo, and isTab) back into the component file or into a single platform-dock-layout.util.ts file that encompasses all the new utility files to avoid having to export RCDockTabInfo and TabType?

done, moved into a spearate file docking-framework-internal.model.ts, but also reduced 3 others into platform-dock-layout-storage.util.ts

---

src/renderer/components/docking/platform-dock-layout-sizing.tsx line 1 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

These new utility files need .util.ts at the end instead of .tsx: as mentioned in the style guide "Filenames and paths" section, we need a type on the filename. Additionally, .tsx is used for React component files. These new files should hopefully be able to be changed into .ts files unless I'm missing something.

done with splitting out createRCDockTabFromTabInfo into platform-dock-tab.component.tsx

---

src/renderer/components/docking/platform-dock-layout-sizing.tsx line 10 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Here and in many places in this PR, please replace these relative paths with path aliases as mentioned in the "Prefer path aliases for imports" line of the style guide.

For example, here, this import would instead be the following:

import DIALOGS from '@renderer/components/dialogs';

done

---

src/renderer/components/docking/platform-dock-layout-wrapper.component.tsx line 9 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

It may be best to use the OnLayoutChangeRCDock type here for consistency.

This was not working with

Code snippet:

Type '(...args: any[]) => void' is not assignable to type 'OnLayoutChangeRCDock'.
  Type 'void' is not assignable to type 'Promise<void>'.ts(2322)

---

src/renderer/components/docking/platform-dock-layout-wrapper.component.tsx line 20 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Though I am not familiar with it, I imagine LegacyRef likely shouldn't be used anymore. Have you tried ForwardedRef?

Here's the TypeScript React cheetsheet page on forwardRef for reference: https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/forward_and_create_ref/

done

[Sebastian-ubs]

[tjcouch-sil]

Reviewed 5 of 11 files at r2, 19 of 19 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Sebastian-ubs)

---

src/renderer/components/docking/platform-dock-layout-wrapper.component.tsx line 9 at r1 (raw file):

Previously, Sebastian-ubs wrote…

This was not working with

Note to self: we realized OnLayoutChangeRCDock isn't quite the same signature as this because it's an async function. So we just pulled straight from rc-dock here since that's what this prop is literally doing. We can change this in the future if needed.

---

src/stories/mui-basics/snackbar.stories.tsx line 2 at r4 (raw file):

import type { Meta, StoryObj } from '@storybook/react';
import { Snackbar, Button } from 'papi-components';

I see a number of stories got moved into a mui-basics folder. However, these files are not related to MUI (MUI happens to be an implementation detail at the current moment, but it is not necessarily going to be forever); these components are from papi-components. I recommend changing the name. Basics works fine. Or papi-components. Or just looping them in with platform. I don't know the relationship between this folder name and anything that displays on storybook, though, so please choose as you see fit :)

[Sebastian-ubs]

Reviewable status: 15 of 26 files reviewed, 3 unresolved discussions (waiting on @tjcouch-sil)

---

src/stories/mui-basics/snackbar.stories.tsx line 2 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I see a number of stories got moved into a mui-basics folder. However, these files are not related to MUI (MUI happens to be an implementation detail at the current moment, but it is not necessarily going to be forever); these components are from papi-components. I recommend changing the name. Basics works fine. Or papi-components. Or just looping them in with platform. I don't know the relationship between this folder name and anything that displays on storybook, though, so please choose as you see fit :)

That's to group them according to how they are named inside storybook (title). I've just changed this to "basics" now. This should include all basic components. We will for sure add more advanced components like dialogs or reusable layouts over time. We may event want to be able to add parts from extensions, but that's something to think about later where to host it so we can put as much components as possible together into this storybook as a workplace to develop and showcase components.

[tjcouch-sil]

thanks for the persistence and hard work on this!

Reviewed 11 of 11 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

src/stories/mui-basics/snackbar.stories.tsx line 2 at r4 (raw file):

Previously, Sebastian-ubs wrote…

That's to group them according to how they are named inside storybook (title). I've just changed this to "basics" now. This should include all basic components. We will for sure add more advanced components like dialogs or reusable layouts over time. We may event want to be able to add parts from extensions, but that's something to think about later where to host it so we can put as much components as possible together into this storybook as a workplace to develop and showcase components.

Great :) thank you!