Mobile: Add notes-bar #6772

Tolu-Mals · 2022-08-20T16:24:49Z

Add Notesbar component
Add NoteActions (button that triggers the notesbar)

Note:
The button to trigger split layout has already been added, but is not yet configured, that's the next part of the project.

laurent22 · 2022-08-20T16:37:56Z

The PR seems to include code from your previous PR? Please rebase using the latest dev branch

Tolu-Mals · 2022-08-20T16:56:29Z

Yes it does. Alright.

packages/app-mobile/components/NotesBar.tsx

packages/app-mobile/components/NotesBarListItem.tsx

personalizedrefrigerator · 2022-08-20T21:21:40Z

packages/app-mobile/components/NotesBar.tsx

+import Folder from '@joplin/lib/models/Folder';
+
+interface Props {
+ themeId: string;


Suggested change

themeId: string;

themeId: number;

It looks like themeStyle takes a number!

It's a bit tricky, but themeStyle is also defined here in global-style.js, there's another themeStyle function defined in theme.ts (The one you referred to), but that's not the function used here. The themeStyle defined in global-style.js ultimately plugs in the theme paramater into themeById function from theme.ts, which takes in a string.

I refactored global-style.js into typescript in this pr, I made the mistake of making themeStyle's parameter a number at first, but corrected it after. I hope that all makes sense 😅

It's a number, or it should be one. How is it in other places in the codebase?

It's not typed in other parts of the codebase.

It should be a string, because the parameter is being passed to themeById function (whose parameter is of type string). It's being passed into themeById here.

That's a mistake in theme.ts then. You can see that eg "Setting.THEME_LIGHT" is a number so theme ID should be a number.

Oh. alright

packages/app-mobile/components/NotesBarListItem.tsx

packages/app-mobile/components/checkbox.tsx

Tolu-Mals · 2022-08-21T12:43:45Z

I messed up the rebase, I'll give it another try.

Tolu-Mals · 2022-08-21T16:24:22Z

The PR seems to include code from your previous PR? Please rebase using the latest dev branch

I just noticed you mentioned latest dev branch here. Did you mean the latest tl-sidemenu-fix branch? I needed to use the getResponsiveValue function from the tl-sidemenu-fix branch in this branch, that's why I merged tl-sidemenu-fix into this branch.

laurent22 · 2022-08-21T20:48:15Z

I just noticed you mentioned latest dev branch here. Did you mean the latest tl-sidemenu-fix branch? I needed to use the getResponsiveValue function from the tl-sidemenu-fix branch in this branch, that's why I merged tl-sidemenu-fix into this branch.

Right sorry, I thought I had merge your other branch. I'm going to give it a try in the coming days then merge, and we can then rebase this pr

Tolu-Mals · 2022-08-22T08:04:31Z

I just noticed you mentioned latest dev branch here. Did you mean the latest tl-sidemenu-fix branch? I needed to use the getResponsiveValue function from the tl-sidemenu-fix branch in this branch, that's why I merged tl-sidemenu-fix into this branch.

Right sorry, I thought I had merge your other branch. I'm going to give it a try in the coming days then merge, and we can then rebase this pr

Okay, alright.

laurent22 · 2022-08-25T16:00:47Z

Many conflicts after the recent merges. Also please rebase with dev so as to pull the changes from your other pull request (now merged).

Tolu-Mals · 2022-08-26T09:55:30Z

Many conflicts after the recent merges. Also please rebase with dev so as to pull the changes from your other pull request (now merged).

Alright, I'll work on that.

laurent22 · 2022-08-26T15:51:57Z

Please don't forget to fix the conflicts.

In the commit that refactored global-style to typescript, I used the wrong type for the parameter of themeStyle(), I fix that with this commit. I also change require() to an import statement to allow typechecking.

This is a fix to the commit that refactored global-style.js into typescript. The fontId parameter in editorFont() and theme parameter in themeStyle() should actually be optional parameteters. You can tell by the logic in those functions.

The module.exports syntax doesn't seem to be working with typescript.

To allow type checking

I needed to use JSX.Element as a type in NotesBar.tsx, but eslint was throwing a 'JSX is undefined error'.

During the refactor of checkbox into typescript, I made the checkbox component a default export. This commit updates all the imports of checbox.js.

personalizedrefrigerator

Nice work so far! I've left a few comments.

A quick note that, if possible, please avoid force-pushing (merge commits are preferable to force-pushes after rebases). Commits will be squashed when merging, so it's okay if the commit history has merge commits.

packages/app-mobile/tsconfig.json

packages/app-mobile/root.tsx

personalizedrefrigerator · 2022-09-01T07:57:46Z

After testing locally, I'm noticing that clicking on the "show notes list" button does nothing:

jop-clicking-noteslist-seems-to-be-a-no-op.mp4

Is this expected?

Edit: Making the following change fixes the issue (even if it does break other things):

diff --git a/packages/app-mobile/components/screens/Note.tsx b/packages/app-mobile/components/screens/Note.tsx
index eec1e3df5..d1cc77b38 100644
--- a/packages/app-mobile/components/screens/Note.tsx
+++ b/packages/app-mobile/components/screens/Note.tsx
@@ -1422,7 +1422,7 @@ class NoteScreenComponent extends BaseScreenComponent {
 
 		// Note actions are the notesbar and split layout toggle button
 		const noteActionButtonGroupComp = (
-			<Animated.View style={this.styles().noteActionButtonGroup} {...this.noteActionsDragResponder.panHandlers} >
+			<Animated.View style={this.styles().noteActionButtonGroup} >
 				{/* Temporarily hiding the split layout button till it's implemented */}
 				{/* <TouchableOpacity style={[this.styles().noteActionButton, this.styles().noteActionButton1]} activeOpacity={0.7}>
 					<Icon name="columns" style={this.styles().noteActionButtonIcon} />

See also https://stackoverflow.com/questions/35814484/react-native-panresponder-elements-clickable

personalizedrefrigerator

I was able to fix it!

packages/app-mobile/components/screens/Note.tsx

packages/app-mobile/components/NotesBar.tsx

personalizedrefrigerator · 2022-09-01T08:19:19Z

packages/app-mobile/components/NotesBar.tsx

+ if (query) {
+ if (props.settings['db.ftsEnabled']) {
+ notes_ = await SearchEngineUtils.notesForQuery(query, true);
+ } else {
+ const p = query.split(' ');
+ const temp = [];
+ for (let i = 0; i < p.length; i++) {
+ const t = p[i].trim();
+ if (!t) continue;
+ temp.push(t);
+ }
+
+ notes_ = await Note.previews(null, {
+ anywherePattern: `*${temp.join('*')}*`,
+ });
+ }


This is almost the same as in search.js. Can this be factored out?

search.js is a class component, so no.

What? Of course you need to refactor this, we can't have duplicate code like this.

I don't think it can be helped in this case. The code in search.js uses this.props as opposed to props because it's a class component. There are also lines of code that are specific to search.js so it's not really reusable. To create a reusable function I would have to check if 'this.props' exists and then use 'props' instead if it doesn't exist, same for 'this.setState', there's no way to tell how the setState function in the function component will be named, since it could be named anything. The function would be really messy. I think it's best left as is.

The code in search.js uses this.props as opposed to props because it's a class component.

Do you seriously think there's no way to solve this? I'm not asking this to bother you - refactoring code is very important to avoid duplicate code all over the codebase. You're not the first one to do that, and if we were to allow this, the codebase would be unmaintainable - any small change we make somewhere will have to copied and pasted in many other places (which of course won't be done).

In this particular case, all you need to do is move the code from search.js to a separate function, replace this.props.xxx with function arguments, make the function return the notes. Call it from search.js and from your new code. setState and so on stay in the component.

I actually thought there was no way to solve this. Thanks, I'll work on fixing it.

packages/app-mobile/components/NotesBar.tsx

personalizedrefrigerator · 2022-09-01T08:31:31Z

I'm noticing that the whole screen flashes when I change notes using the notes bar. Is there a way to handle changing notes without destroying/recreating the notes bar (e.g. creating a new NotesScreenContent component that does refresh, while Note.tsx doesn't — perhaps this would be best left to a different PR, however.)?

Co-authored-by: Henry Heino <[email protected]>

Removed the any types in onPanResponderMove, so that it can be automatically inferred. Added a whitespace because it looks better.

laurent22 · 2022-09-01T10:18:22Z

I'm noticing that the whole screen flashes when I change notes using the notes bar. Is there a way to handle changing notes without destroying/recreating the notes bar (e.g. creating a new NotesScreenContent component that does refresh, while Note.tsx doesn't — perhaps this would be best left to a different PR, however.)?

Hmm, yes maybe a different PR would be fine. I guess Tolu will need to check what props the note bar receive - maybe some of them aren't needed and are triggering an unnecessary re-render.

laurent22 · 2022-09-01T10:56:46Z

@Tolu-Mals, if you could address personalizedrefrigerator's comments I think we can merge.

Co-authored-by: Henry Heino <[email protected]>

…o tl-notesbar

Co-authored-by: Henry Heino <[email protected]>

Tolu-Mals · 2022-09-01T12:32:25Z

I'm noticing that the whole screen flashes when I change notes using the notes bar. Is there a way to handle changing notes without destroying/recreating the notes bar (e.g. creating a new NotesScreenContent component that does refresh, while Note.tsx doesn't — perhaps this would be best left to a different PR, however.)?

So I checked onJoplinLinkClick_ in the Note screen which handles navigating to another note when a Joplin note link is clicked and I saw that the Note screen doesn't handle reloading a different note, but a workaround is to go back first before navigating to another note. I followed that pattern, that's why there's the screen flash.

onJoplinLinkClick_

@laurent22 @personalizedrefrigerator

packages/app-mobile/components/NotesBar.tsx

laurent22 · 2022-09-06T14:36:18Z

packages/app-mobile/components/HandleNoteQuery.ts

+// Be sure to use 'await' keyword or the function might not work properly
+// Eg. await HandleNoteQuery();
+
+const HandleNoteQuery = async (query: string, settings: any, dispatch: (action: Object)=> void): Promise<any[]> => {


Maybe call it searchNotes? (and rename to searchNotes.ts too)

Oh, alright.

laurent22 · 2022-09-06T14:36:27Z

packages/app-mobile/components/HandleNoteQuery.ts

+// Be sure to use 'await' keyword or the function might not work properly
+// Eg. await HandleNoteQuery();
+
+const HandleNoteQuery = async (query: string, settings: any, dispatch: (action: Object)=> void): Promise<any[]> => {


Should return NoteEntity[] I believe?

laurent22 · 2022-09-06T14:38:07Z

packages/app-mobile/components/HandleNoteQuery.ts

+// Be sure to use 'await' keyword or the function might not work properly
+// Eg. await HandleNoteQuery();
+
+const HandleNoteQuery = async (query: string, settings: any, dispatch: (action: Object)=> void): Promise<any[]> => {


Pass only dbFtsEnabled (boolean), not the whole settings array. In general you should only pass what's strictly necessary to a function, which makes it easier to test it and use it.

Noted, I'll fix that.

laurent22 · 2022-09-06T15:59:08Z

There's a failing test here: https://github.com/laurent22/joplin/runs/8208783103?check_suite_focus=true#step:10:2721

Are you able to replicate the failure on your computer?

Tolu-Mals · 2022-09-07T08:03:46Z

There's a failing test here: https://github.com/laurent22/joplin/runs/8208783103?check_suite_focus=true#step:10:2721

Are you able to replicate the failure on your computer?

The tests are all passing on my computer. But I've noticed that particular test sometimes fails, and then passes again.

laurent22 · 2022-09-07T09:02:32Z

@personalizedrefrigerator, it seems that particular test is flaky, do you know what could be the issue? Perhaps the test is sometimes started before the editor is fully initialised?

https://github.com/laurent22/joplin/runs/8208783103?check_suite_focus=true#step:10:2721

Renamed HandleNoteQuery to searchNotes.ts, also passed only the dbFtsEnabled argument instead of the entire settings object.

laurent22 · 2022-09-09T14:05:54Z

Ok let's merge. Thanks @Tolu-Mals, and thanks @personalizedrefrigerator for the review!

Revert commit dfd95f8 Due to UX issues. Ref https://discourse.joplinapp.org/t/25775/30

personalizedrefrigerator reviewed Aug 20, 2022

View reviewed changes

Tolu-Mals force-pushed the tl-notesbar branch from 2a6945a to 3bda937 Compare August 21, 2022 06:57

Tolu-Mals force-pushed the tl-notesbar branch from 3bda937 to 2a6945a Compare August 22, 2022 14:26

Tolu-Mals force-pushed the tl-notesbar branch from cf2150c to 476a1c8 Compare August 26, 2022 14:38

Tolu-Mals added 16 commits August 26, 2022 19:28

Refactor global-style.js into typescript

f78ba7b

Fix parameter type, change require to import

eaab3a8

In the commit that refactored global-style to typescript, I used the wrong type for the parameter of themeStyle(), I fix that with this commit. I also change require() to an import statement to allow typechecking.

Make fontId and theme parameters optional

03b4d65

This is a fix to the commit that refactored global-style.js into typescript. The fontId parameter in editorFont() and theme parameter in themeStyle() should actually be optional parameteters. You can tell by the logic in those functions.

Add parameter type to connect()

491b7fe

Change 'module.export..' syntax to 'export function...'

d0a61f5

The module.exports syntax doesn't seem to be working with typescript.

Change require to import

6fc6e4d

To allow type checking

Add notesbar 1

2e512ee

Add notesbar 2

13ecb86

Add notesbar 3, add JSX to eslint globals

69d5573

I needed to use JSX.Element as a type in NotesBar.tsx, but eslint was throwing a 'JSX is undefined error'.

Add notesbar 4

a1d2368

Add notesbar 5

c39d30c

Add notesbar 6

2336d6f

Refactor checkbox.js into typescript

87f0187

Change checkbox.js imports to default imports

fd69dd5

During the refactor of checkbox into typescript, I made the checkbox component a default export. This commit updates all the imports of checbox.js.

Refactor checkbox.js to typescript 2, change checkbox.js imports 2

f7f5aef

Add notesbar 7

133a808

personalizedrefrigerator reviewed Sep 1, 2022

View reviewed changes

packages/app-mobile/tsconfig.json Outdated Show resolved Hide resolved

packages/app-mobile/root.tsx Show resolved Hide resolved

packages/app-mobile/root.tsx Outdated Show resolved Hide resolved

personalizedrefrigerator reviewed Sep 1, 2022

View reviewed changes

packages/app-mobile/components/screens/Note.tsx Outdated Show resolved Hide resolved

packages/app-mobile/components/screens/Note.tsx Show resolved Hide resolved

packages/app-mobile/components/screens/Note.tsx Outdated Show resolved Hide resolved

personalizedrefrigerator reviewed Sep 1, 2022

View reviewed changes

packages/app-mobile/components/NotesBar.tsx Outdated Show resolved Hide resolved

personalizedrefrigerator reviewed Sep 1, 2022

View reviewed changes

packages/app-mobile/components/NotesBar.tsx Outdated Show resolved Hide resolved

personalizedrefrigerator reviewed Sep 1, 2022

View reviewed changes

packages/app-mobile/components/NotesBar.tsx Outdated Show resolved Hide resolved

Tolu-Mals and others added 3 commits September 1, 2022 10:22

Change require to import to support typing

a451475

Co-authored-by: Henry Heino <[email protected]>

Fix notesbar toggle button bug

0fd4a09

Co-authored-by: Henry Heino <[email protected]>

Fix typing, small change

c0c5bce

Removed the any types in onPanResponderMove, so that it can be automatically inferred. Added a whitespace because it looks better.

Fix notesbar toggle button bug

cfd4525

Tolu-Mals and others added 4 commits September 1, 2022 12:00

Add accessibility label to button

d0342d5

Co-authored-by: Henry Heino <[email protected]>

Merge branch 'tl-notesbar' of https://github.com/Tolu-Mals/joplin int…

4d87003

…o tl-notesbar

Add a11y button and role to renderIconButton

f72af41

Refactor handleQuerySubmit

08408f6

Co-authored-by: Henry Heino <[email protected]>

laurent22 reviewed Sep 5, 2022

View reviewed changes

packages/app-mobile/components/NotesBar.tsx Outdated Show resolved Hide resolved

packages/app-mobile/components/NotesBar.tsx Outdated Show resolved Hide resolved

Tolu-Mals added 2 commits September 5, 2022 15:30

Refactor NotesBar

aeca65d

Refactor search logic in Notesbar component and search screen component

1a67fd9

laurent22 reviewed Sep 6, 2022

View reviewed changes

Refactor note search logic

b255987

Renamed HandleNoteQuery to searchNotes.ts, also passed only the dbFtsEnabled argument instead of the entire settings object.

laurent22 merged commit dfd95f8 into laurent22:dev Sep 9, 2022

laurent22 added a commit that referenced this pull request Sep 30, 2022

Revert: Mobile: Add note bar (#6772)

44e60bd

Revert commit dfd95f8 Due to UX issues. Ref https://discourse.joplinapp.org/t/25775/30

Mobile: Add notes-bar #6772

Mobile: Add notes-bar #6772

Conversation

Tolu-Mals commented Aug 20, 2022

laurent22 commented Aug 20, 2022

Tolu-Mals commented Aug 20, 2022

Choose a reason for hiding this comment

Tolu-Mals Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tolu-Mals Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tolu-Mals commented Aug 21, 2022

Tolu-Mals commented Aug 21, 2022 • edited Loading

laurent22 commented Aug 21, 2022

Tolu-Mals commented Aug 22, 2022

laurent22 commented Aug 25, 2022

Tolu-Mals commented Aug 26, 2022

laurent22 commented Aug 26, 2022

personalizedrefrigerator left a comment

Choose a reason for hiding this comment

personalizedrefrigerator commented Sep 1, 2022 • edited Loading

personalizedrefrigerator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

personalizedrefrigerator commented Sep 1, 2022 • edited Loading

laurent22 commented Sep 1, 2022

laurent22 commented Sep 1, 2022

Tolu-Mals commented Sep 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented Sep 6, 2022

Tolu-Mals commented Sep 7, 2022 • edited Loading

laurent22 commented Sep 7, 2022 • edited Loading

laurent22 commented Sep 9, 2022

Tolu-Mals Aug 23, 2022 •

edited

Loading

Tolu-Mals Aug 23, 2022 •

edited

Loading

Tolu-Mals commented Aug 21, 2022 •

edited

Loading

personalizedrefrigerator commented Sep 1, 2022 •

edited

Loading

personalizedrefrigerator commented Sep 1, 2022 •

edited

Loading

Tolu-Mals commented Sep 7, 2022 •

edited

Loading

laurent22 commented Sep 7, 2022 •

edited

Loading