-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Mobile: Add notes-bar #6772
Mobile: Add notes-bar #6772
Conversation
The PR seems to include code from your previous PR? Please rebase using the latest dev branch |
Yes it does. Alright. |
import Folder from '@joplin/lib/models/Folder'; | ||
|
||
interface Props { | ||
themeId: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
themeId: string; | |
themeId: number; |
It looks like themeStyle
takes a number!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a number, or it should be one. How is it in other places in the codebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. alright
I messed up the rebase, I'll give it another try. |
I just noticed you mentioned latest dev branch here. Did you mean the latest tl-sidemenu-fix branch? I needed to use the |
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. |
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. |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.mp4Is 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to fix it!
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('*')}*`, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost the same as in search.js
. Can this be factored out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
search.js
is a class component, so no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What? Of course you need to refactor this, we can't have duplicate code like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually thought there was no way to solve this. Thanks, I'll work on fixing it.
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 |
Co-authored-by: Henry Heino <[email protected]>
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.
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. |
@Tolu-Mals, if you could address personalizedrefrigerator's comments I think we can merge. |
So I checked |
// 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[]> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call it searchNotes? (and rename to searchNotes.ts too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, alright.
// 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[]> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should return NoteEntity[] I believe?
// 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[]> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, I'll fix that.
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. |
@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.
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
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.