Jump to conversation
Unresolved conversations (0)
Nice work!

Nice work!

All of your conversations have been resolved.

Resolved conversations (22)
@laurent22 laurent22 Mar 6, 2021
Sorry but it's still not quite right. I wrote this before: > please use proper types. You can find them in interface AppState and also this before: > We need to define what arguments this function takes, so please either create an interface for props that tells what props are actually used by the function, or create separate arguments for each dependency. But unfortunately you've changed it again and you're now passing the whole application state to the function, making it impossible to know what arguments the function actually needs. As a result, in the tests you are passing "any" for all the variables, so we have no type checks whatsoever. It would help if you could follow my suggestions or if you think what I'm saying makes no sense, let's discuss it, but there's no point changing the code over and over, each new version being as incorrect as the one before.
Outdated
packages/app-desktop/gui/getWindowTitle.ts
asrient laurent22
@laurent22 laurent22 Feb 23, 2021
`export default function ....`
Outdated
packages/app-desktop/gui/getWindowTitle.ts
@laurent22 laurent22 Feb 23, 2021
Please remove
Outdated
packages/app-desktop/gui/getWindowTitle.ts
@laurent22 laurent22 Feb 23, 2021
`const getWindowTitle = require('./getWindowTitle').default;`
Outdated
packages/app-desktop/gui/Navigator.jsx
@laurent22 laurent22 Feb 23, 2021
`import getWindowTitle from ./getWindowTitle'`
Outdated
...es/app-desktop/gui/getWindowTitle.test.ts
@laurent22 laurent22 Feb 23, 2021
no need to trim
Outdated
packages/app-desktop/gui/getWindowTitle.ts
@laurent22 laurent22 Feb 23, 2021
No need to trim
Outdated
packages/app-desktop/gui/getWindowTitle.ts
@laurent22 laurent22 Feb 23, 2021
please use proper types. You can find them in interface AppState
Outdated
packages/app-desktop/gui/getWindowTitle.ts
@laurent22 laurent22 Feb 23, 2021
Please remove
...es/app-desktop/gui/getWindowTitle.test.ts
@laurent22 laurent22 Feb 23, 2021
Please remove. This directory is now called "Sidebar".
Outdated
.gitignore
@laurent22 laurent22 Feb 23, 2021
Please remove
Outdated
.eslintignore
@laurent22 laurent22 Jan 27, 2021
Please narrow down the data to just what's needed by getWindowTitle. Also please test other conditions: - No selected notes - No selected folders - When we're not on the Main screen By simplifying the input data it's easier to create multiple tests and they will also be easier to maintain.
Outdated
...es/app-desktop/gui/getWindowTitle.test.ts
@laurent22 laurent22 Jan 27, 2021
We need to define what arguments this function takes, so please either create an interface for `props` that tells what props are actually used by the function, or create separate arguments for each dependency.
Outdated
packages/app-desktop/gui/getWindowTitle.ts
@laurent22 laurent22 Jan 20, 2021
We need unit test for this function. Perhaps one way would be to move the function to a separate file `getWindowTitle.ts` and have `getWindowTitle.test.ts` next to it. Please see how other test files are setup within the app-desktop package for more information, and if you need any help let me know.
Outdated
packages/app-desktop/gui/Navigator.jsx
asrient
@laurent22 laurent22 Jan 20, 2021
`_('Untitled')`
Outdated
packages/app-desktop/gui/Navigator.jsx
@laurent22 laurent22 Jan 20, 2021
Please shorten to this: ``` const note = props.selectedNoteIds.length ? BaseModel.byId(props.notes, props.selectedNoteIds[0]) : null const folder = props.selectedFolderId ? BaseModel.byId(props.folders, selectedFolderId) : null ```
Outdated
packages/app-desktop/gui/Navigator.jsx
@laurent22 laurent22 Jan 20, 2021
Rename to "props"
Outdated
packages/app-desktop/gui/Navigator.jsx
@laurent22 laurent22 Jan 20, 2021
Any reason for this "prevTitle" optimisation? Is it slow without it?
Outdated
packages/app-desktop/gui/Navigator.jsx
laurent22 asrient
@laurent22 laurent22 Jan 20, 2021
I see there's already a `substrWithEllipsis` function in string-utils so please use this.
Outdated
packages/app-desktop/gui/Navigator.jsx
@laurent22 laurent22 Jan 19, 2021
We don't add derived state to the state because you can get all this at the point where you need it. In fact, please create a "windowTitle()" function that encapsulates that new logic and the existing logic and returns the window title. Having a separate function will also make it easier to unit test it.
Outdated
packages/app-desktop/gui/Navigator.jsx
@laurent22 laurent22 Jan 19, 2021
I would suggest using a breadcrump: `folderTitle > noteTitle`
Outdated
packages/app-desktop/gui/Navigator.jsx
@laurent22 laurent22 Jan 19, 2021
How does it handle long titles?
Outdated
packages/app-desktop/gui/Navigator.jsx
laurent22 asrient