Mobile: Add markdown toolbar #6753

personalizedrefrigerator · 2022-08-16T03:46:18Z

Summary

Adds a markdown toolbar to the mobile app
Feature/design discussion

Screenshots

laurent22 · 2022-08-17T16:12:54Z

I didn't see the discussion for changing the toolbar style? I guess the new one is fine, but you also mentioned that the previous one was closer to what Apple Notes and other apps are doing. What was the reason for changing?

personalizedrefrigerator · 2022-08-17T21:50:06Z

I didn't see the discussion for changing the toolbar style? I guess the new one is fine, but you also mentioned that the previous one was closer to what Apple Notes and other apps are doing. What was the reason for changing?

This implementation is (intended) to be closer to what Apple Notes and other apps are doing. See the discussion in the previous pull request.

laurent22 · 2022-08-18T08:19:00Z

Right I see what you mean. I felt your previous iteration was already a bit similar to Apple Notes, with the opening/closing of panels, but this is indeed even closer. I'm going to install your branch and give it a try.

laurent22 · 2022-08-18T08:23:36Z

packages/app-mobile/components/NoteEditor/MarkdownToolbar.tsx

+ );
+ }
+
+ if (!props.visible) {


Maybe put that at the top of the function? That way you skip the loop if the component is not visible.

laurent22 · 2022-08-18T08:24:35Z

packages/app-mobile/components/NoteEditor/MarkdownToolbar.tsx

+const buttonSize = 56;
+
+// Displays a list of buttons with an overflow menu.
+const Toolbar = (props: ToolbarProps) => {


There's too many of these sub-components. Could you create a MarkdownToolbar folder and move these components to file please?

laurent22 · 2022-08-18T08:29:37Z

packages/app-mobile/components/NoteEditor/MarkdownToolbar.tsx

+ // This hook is executed the same number of times for each call
+ // of MarkdownToolbar.
+ // eslint-disable-next-line react-hooks/rules-of-hooks
+ onClick: useCallback(() => {


Could you move this outside of the loop, and maybe pass the level to it from here? onClick: onHeaderButtonClick({ level }) I'd rather we don't silence the rules of hooks.

If extracted to a different function (both useHeaderButtonClick and onHeaderButtonClick), I get a different rules-of-hooks linter error. I think it makes more sense to silence the warning here without adding an extra function than silencing the warning within the wrapper function.

laurent22 · 2022-08-18T08:54:09Z

packages/app-mobile/components/NoteEditor/MarkdownToolbar.tsx

+ editorControl.searchControl.hideSearch();
+ } else {
+ editorControl.searchControl.showSearch();
+ }


I don't think this editorControl object is well setup because it has hidden dependencies, so you might end up using stale state over there. For example showSearch uses searchState, yet the editorControl is not recreated when searchState is changed.

I'm thinking, rather than passing editorControl to everything, you might want to pass only the specific handler needed by your callback. Then on the editorControl, you define at least the functions that have dependencies as callbacks. So showSearch: useCallback(() => ...., [searchState])

And here you pass editorControl.searchControl.showSearch as dependency.

Edit: now that I see the showSearch code again, I think you should call setSearchState((prev) => { return { ...prev, dialogVisible: true }) and that way you don't need useCallback. But I still think that passing the full editorControl as dependency to everything is not ideal in terms of rendering. Each useCallback should only have a dependency to the specific handler they need.

I've updated this with a searchStateRef = RefObject<SearchState>. As such, neither searchControl nor editorControl need to be re-created when SearchState changes.

laurent22 · 2022-08-18T09:24:19Z

I've checked the UI and it looks pretty good. Just a few observations:

The "on press" colour, which is black now, doesn't look so good. Maybe make it dark or light gray?
There should be a close (X) button on the popup because it might not be obvious you need to click again on the three dots. Also in landscape mode, those three dots might not even be visible (because the panel becomes scrollable)

The way it hide/show buttons depending on the screen width is smart, but I wonder if we really want this. On portrait, it only shows a Bold button for me, which is a bit odd. Like it looks like there's just one formatting option until you try to click on the three dots. On landscape it's better, but then there's a question of why certain buttons, like Math mode (which many users won't need), is displayed, when list buttons (much most commonly used) are not.

Obviously this is all automatic for now, you display based on what's in the array, but I think we should review what's displayed. Perhaps buttons should be assigned a priority to decide which one should be displayed or not depending on available space?

Another thing is that in portrait mode, you actually have more horizontal space so you could display more buttons.

Also what is this keyboard button on the right of the bar? It's grayed out for me. Is it possible to remove it and put the three dots completely on the right? I feel that would make it more obvious that it's a sub menu.

personalizedrefrigerator · 2022-08-18T10:25:10Z

button on the right of the bar? It's grayed out for me. Is it possible to remove it and put the three dots completely on the right?

Thank you for the code review! The greyed-out keyboard shows/hides the software keyboard. I was originally hiding it altogether when the software keyboard was hidden, but I found the resultant change-in-visible-buttons to be potentially-confusing, so am currently just disabling it.

Ideally, the "hide software keyboard" button wouldn't be visible if the user has a hardware keyboard connected. Currently, however, I'm not sure how to check for this...

Another thing is that in portrait mode, you actually have more horizontal space so you could display more buttons.

That's strange... I'm not experiencing this on my Android emulator/tablet. Are you testing this on an emulator/simulator? If so, which device? (I would like to be able to reproduce this :) )

laurent22 · 2022-08-18T10:46:06Z

That's strange... I'm not experiencing this on my Android emulator/tablet. Are you testing this on an emulator/simulator? If so, which device? (I would like to be able to reproduce this :) )

Yes it's on the simulator, I'm using iPhone 13 for the above test. In the screenshot as you can see there are three buttons, when there would be room for 6 or 7 buttons.

when searchState changes This allows the structure of [editorControl] to continue to match the structure of the [cm] object exported by [CodeMirror/CodeMirror.ts].

…' into pr/markdownToolbar2

personalizedrefrigerator · 2022-08-19T13:00:07Z

It looks like the CI errors are related to the desktop app:

   Error: ➤ YN0000: │ root@workspace:. STDERR ➤ YN0000: [@joplin/app-desktop]: app.ts(38,32): error TS2306: File '/home/runner/work/joplin/joplin/packages/app-desktop/gui/MainScreen/commands/index.ts' is not a module.
  Error: ➤ YN0000: │ root@workspace:. STDERR ➤ YN0000: [@joplin/app-desktop]: gui/MainScreen/MainScreen.tsx(41,22): error TS2306: File '/home/runner/work/joplin/joplin/packages/app-desktop/gui/MainScreen/commands/index.ts' is not a module.

https://github.com/laurent22/joplin/runs/7918363808?check_suite_focus=true#step:6:4436

…d/replaced with something else

personalizedrefrigerator · 2022-08-20T05:20:05Z

The "on press" colour, which is black now, doesn't look so good. Maybe make it dark or light gray?

I've made them TouchableOpacity components instead of TouchableHighlight components:

There should be a close (X) button on the popup because it might not be obvious you need to click again on the three dots. Also in landscape mode, those three dots might not even be visible (because the panel becomes scrollable)

This has been added!

The way it hide/show buttons depending on the screen width is smart, but I wonder if we really want this. On portrait, it only shows a Bold button for me, which is a bit odd. Like it looks like there's just one formatting option until you try to click on the three dots. On landscape it's better, but then there's a question of why certain buttons, like Math mode (which many users won't need), is displayed, when list buttons (much most commonly used) are not.

I've re-prioritized the buttons. (If math/TeX is disabled in settings, the ∑ button is invisible.)

Another thing is that in portrait mode, you actually have more horizontal space so you could display more buttons.

The logic computing the available space for buttons should now be fixed!

Additionally, I think that the spellcheck toggle button should be removed (and spellcheck enabled by default). With this removed, however, the "actions" row of the menu has one less item than the other rows. What action might I put here instead? (Perhaps an 'attach' button? A 'toggle rich markdown mode' button?)

personalizedrefrigerator · 2022-08-21T21:32:36Z

Additionally, I think that the spellcheck toggle button should be removed (and spellcheck enabled by default). With this removed, however, the "actions" row of the menu has one less item than the other rows. What action might I put here instead? (Perhaps an 'attach' button? A 'toggle rich markdown mode' button?)

I've enabled spellchecking by default. While it works well on Android, it's quite buggy on iOS (suggestions appear after tapping on a misspelled word, but red underlines aren't visible). I think this is further reason to remove the spellcheck button...

I'm currently leaning towards an 'attach' button like that in the existing dropdown menu. Another option would be a 'jump to start'/'jump to end' button.

laurent22 · 2022-08-27T12:34:40Z

Thanks for the update. Is it not possible to remove that keyboard button? I feel it's not important enough to be always visible there as I don't expect most people use a keyboard with their phone.

personalizedrefrigerator · 2022-08-27T14:22:56Z

Thanks for the update. Is it not possible to remove that keyboard button? I feel it's not important enough to be always visible there as I don't expect most people use a keyboard with their phone.

This should be fixed!

Additional notes/explanation

The keyboard button is intended for people who don't use a hardware keyboard with their phone — it hides the software keyboard.

I found out how to hide the keyboard button when a software keyboard hasn't been shown, however! As such, the "hide software keyboard" button is now only visible when a software keyboard has been shown at least once.

Note: I'm disabling and not removing the "hide software keyboard" button when the keyboard isn't visible. If the "hide software keyboard" button were removed when no software keyboard is visible, the number/position of the toolbar buttons would change (which, for a given screen size, I would like to avoid).

laurent22 · 2022-08-29T10:00:36Z

The keyboard button is intended for people who don't use a hardware keyboard with their phone — it hides the software keyboard.

Hmm, but why is it needed? Previously we had no such button and I guess people just used the built-in keyboard dismiss button (at least on Android), or the keyboard would automatically be dismissed when the note is closed. When they are in editing mode anyway I don't think a software dismiss button is particularly useful. The fact that no-one ever asked for it is a good indication that it's not really needed.

laurent22 · 2022-08-29T10:01:21Z

I don't know if you can replicate this but on my simulator the keyboard is overlapping the bottom of the toolbar. Maybe some extra padding is needed when the keyboard is visible?

personalizedrefrigerator · 2022-08-29T10:26:23Z

I don't know if you can replicate this but on my simulator the keyboard is overlapping the bottom of the toolbar. Maybe some extra padding is needed when the keyboard is visible?

I'm not noticing this on Android. I do see iOS-specific logic for adding additional padding elsewhere, however:

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

Lines 1126 to 1128 in e784e8c

 // need some extra padding for iOS so that the keyboard won't cover last line of the note 

 // see https://github.com/laurent22/joplin/issues/3607 

 paddingBottom={ Platform.OS === 'ios' ? 40 : 0}

I'll do this for the note editor, too!

Edit: It happens on Android, too, but much less than on iOS. Both should be fixed now.

tomasz1986 · 2022-08-29T10:44:51Z

Hmm, but why is it needed? Previously we had no such button and I guess people just used the built-in keyboard dismiss button (at least on Android), or the keyboard would automatically be dismissed when the note is closed. When they are in editing mode anyway I don't think a software dismiss button is particularly useful. The fact that no-one ever asked for it is a good indication that it's not really needed.

I think a reason for the button is that the software keyboard and the toolbar together can basically cover the whole screen on smaller devices, hence rendering input extremely difficult. There's no built-in dismiss keyboard button on (stock) Android, is there? The back button should normally dismiss the keyboard though.

@personalizedrefrigerator What happens if you click the back button with both the software keyboard and the toolbar visible? Does it hide both of them or only the keyboard?

personalizedrefrigerator · 2022-08-29T10:59:01Z

I don't know if you can replicate this but on my simulator the keyboard is overlapping the bottom of the toolbar. Maybe some extra padding is needed when the keyboard is visible?

Done!

What happens if you click the back button with both the software keyboard and the toolbar visible? Does it hide both of them or only the keyboard?

On Android, it only hides the keyboard.

The reason I added the hide keyboard button is because I removed the built-in "hide keyboard" button on iOS for extra vertical space.

This is also why the "hide keyboard" button is on the far right side of the toolbar -- to match the built-in toolbar.

laurent22 · 2022-08-29T11:10:07Z

The reason I added the hide keyboard button is because I removed the built-in "hide keyboard" button on iOS for extra vertical space.

That makes sense, and sounds like a good change on iOS then.

On Android I think keyboards always have a built-in dismiss button though, like below. In which case perhaps we can remove it on Android.

personalizedrefrigerator · 2022-08-29T11:15:06Z

The reason I added the hide keyboard button is because I removed the built-in "hide keyboard" button on iOS for extra vertical space.

That makes sense, and sounds like a good change on iOS then.

On Android I think keyboards always have a built-in dismiss button though, like below. In which case perhaps we can remove it on Android.

~~I think I received a request to keep it on Android: https://discourse.joplinapp.org/t/mobile-markdown-toolbar/26089/31?u=personalizedrefriger~~

I've removed it on Android!

…' into pr/markdownToolbar2

tomasz1986 · 2022-08-29T11:25:40Z

I think I received a request to keep it on Android: discourse.joplinapp.org/t/mobile-markdown-toolbar/26089/31?u=personalizedrefriger .

Just a comment, but it's likely that the user wasn't aware that the back button was used to hide the keyboard, which is quite common actually, as the button is known to be quite convoluted (due to acting differently depending on the situation). The functionality itself is present on all Android devices (see https://support.google.com/android/answer/9079644). Not sure if mandatory, but the only case, where there could really be no back button present, would likely be a heavily modified custom ROM or a system with custom UI modifications.

laurent22 · 2022-08-29T11:32:09Z

Brilliant, thanks guys for checking this. Let's wait till CI is done and we can merge.

ccvbc · 2022-08-30T02:40:17Z

I want to share my opinion

When I want to bold and italicize text at the same time, I can't do it in one step, here I recorded a video of joplin and another software.
Whether custom html tags or markdown tags should be added, people cannot always rely on the official update software to add them. For example the "underscore" I want is not there.
I've seen some people disagree with mobile custom tags before, I think it's very useful, joplin users need her, he can m
ake joplin mobile more powerful.
@personalizedrefrigerator

Screenrecording_20220830_103521.mp4

Screenrecording_20220830_101334.mp4

njakes · 2022-09-26T02:15:57Z

Will there be a way to create check boxes with this toolbar?

Daeraxa · 2022-09-26T02:49:32Z

Yes, its in the lists section

globalhuman · 2022-09-27T10:24:47Z

This is a great feature, I was wondering when it would be coming to the next iOS release or is there a TestFlight I am able to join?

personalizedrefrigerator added 2 commits August 15, 2022 20:42

Mobile: Add markdown toolbar

4826e37

Comment: Explain why overflow hidden is being used

7267780

laurent22 reviewed Aug 18, 2022

View reviewed changes

Use useRef instead of useState to prevent re-creation of [editorControl]

d5be10d

when searchState changes This allows the structure of [editorControl] to continue to match the structure of the [cm] object exported by [CodeMirror/CodeMirror.ts].

personalizedrefrigerator marked this pull request as draft August 19, 2022 08:04

laurent22 and others added 4 commits August 19, 2022 12:18

Merge branch 'dev' into pr/markdownToolbar2

49c5847

Fix searchState not syncing between ref and state

6543ca1

Merge remote-tracking branch 'refs/remotes/origin/pr/markdownToolbar2…

54f6240

…' into pr/markdownToolbar2

Refactor MarkdownToolbar.tsx

f4ccb44

personalizedrefrigerator marked this pull request as ready for review August 19, 2022 13:07

laurent22 and others added 8 commits August 19, 2022 19:10

Merge branch 'dev' into pr/markdownToolbar2

12d57f8

Fix un-updated searchStateRef update

78ad620

Add close button

80c5a56

Ensure that there is enough space for the close button

8b5f22f

Shrink toolbar/scroll when there is vertical overflow

5372d8c

Sort buttons in the toolbar by hardcoded priorities

a8471ec

Change button opacity on click instead of background

6c48cab

Decrease priority of spellcheck button --- it will probably be remove…

f74ec06

…d/replaced with something else

personalizedrefrigerator added 3 commits August 21, 2022 14:11

Merge remote-tracking branch 'upstream/dev' into pr/markdownToolbar2

0699d92

Add tooltips to markdown toolbar buttons

9a9e050

Add tooltips to markdown toolbar buttons

1dc421f

personalizedrefrigerator and others added 2 commits August 23, 2022 19:34

Replace spellcheck button with attach button

fab5d06

Merge branch 'dev' into pr/markdownToolbar2

6e40a57

personalizedrefrigerator added 2 commits August 27, 2022 07:09

Merge remote-tracking branch 'upstream/dev' into pr/markdownToolbar2

ce5e4fc

Only show "hide keyboard" button if the software keyboard has been shown

09e77ab

Merge branch 'dev' into pr/markdownToolbar2

d65dbcb

Additional padding on iOS (to-do: test on iOS)

c005afa

Test additional padding on iOS & fix issues with it

c77ec41

personalizedrefrigerator and others added 2 commits August 29, 2022 04:07

Fix toolbar row not tall enough for buttons with borders

af136f2

Merge branch 'dev' into pr/markdownToolbar2

0e77d95

personalizedrefrigerator added 2 commits August 29, 2022 04:19

Remove "hide keyboard" button on Android

8b0b345

Merge remote-tracking branch 'refs/remotes/origin/pr/markdownToolbar2…

1959cd8

…' into pr/markdownToolbar2

laurent22 merged commit b174fcf into laurent22:dev Aug 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mobile: Add markdown toolbar #6753

Mobile: Add markdown toolbar #6753

personalizedrefrigerator commented Aug 16, 2022 •

edited

Loading

laurent22 commented Aug 17, 2022

personalizedrefrigerator commented Aug 17, 2022 •

edited

Loading

laurent22 commented Aug 18, 2022

laurent22 Aug 18, 2022

laurent22 Aug 18, 2022

laurent22 Aug 18, 2022

personalizedrefrigerator Aug 19, 2022

laurent22 Aug 18, 2022 •

edited

Loading

personalizedrefrigerator Aug 19, 2022

laurent22 commented Aug 18, 2022

personalizedrefrigerator commented Aug 18, 2022 •

edited

Loading

laurent22 commented Aug 18, 2022

personalizedrefrigerator commented Aug 19, 2022 •

edited

Loading

personalizedrefrigerator commented Aug 20, 2022 •

edited

Loading

personalizedrefrigerator commented Aug 21, 2022

laurent22 commented Aug 27, 2022

personalizedrefrigerator commented Aug 27, 2022 •

edited

Loading

laurent22 commented Aug 29, 2022

laurent22 commented Aug 29, 2022

personalizedrefrigerator commented Aug 29, 2022 •

edited

Loading

tomasz1986 commented Aug 29, 2022

personalizedrefrigerator commented Aug 29, 2022 •

edited

Loading

laurent22 commented Aug 29, 2022

personalizedrefrigerator commented Aug 29, 2022 •

edited

Loading

tomasz1986 commented Aug 29, 2022 •

edited

Loading

laurent22 commented Aug 29, 2022

ccvbc commented Aug 30, 2022

njakes commented Sep 26, 2022

Daeraxa commented Sep 26, 2022

globalhuman commented Sep 27, 2022

Mobile: Add markdown toolbar #6753

Mobile: Add markdown toolbar #6753

Conversation

personalizedrefrigerator commented Aug 16, 2022 • edited Loading

Summary

Screenshots

laurent22 commented Aug 17, 2022

personalizedrefrigerator commented Aug 17, 2022 • edited Loading

laurent22 commented Aug 18, 2022

laurent22 Aug 18, 2022

Choose a reason for hiding this comment

laurent22 Aug 18, 2022

Choose a reason for hiding this comment

laurent22 Aug 18, 2022

Choose a reason for hiding this comment

personalizedrefrigerator Aug 19, 2022

Choose a reason for hiding this comment

laurent22 Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

personalizedrefrigerator Aug 19, 2022

Choose a reason for hiding this comment

laurent22 commented Aug 18, 2022

personalizedrefrigerator commented Aug 18, 2022 • edited Loading

laurent22 commented Aug 18, 2022

personalizedrefrigerator commented Aug 19, 2022 • edited Loading

personalizedrefrigerator commented Aug 20, 2022 • edited Loading

personalizedrefrigerator commented Aug 21, 2022

laurent22 commented Aug 27, 2022

personalizedrefrigerator commented Aug 27, 2022 • edited Loading

Additional notes/explanation

laurent22 commented Aug 29, 2022

laurent22 commented Aug 29, 2022

personalizedrefrigerator commented Aug 29, 2022 • edited Loading

tomasz1986 commented Aug 29, 2022

personalizedrefrigerator commented Aug 29, 2022 • edited Loading

laurent22 commented Aug 29, 2022

personalizedrefrigerator commented Aug 29, 2022 • edited Loading

tomasz1986 commented Aug 29, 2022 • edited Loading

laurent22 commented Aug 29, 2022

ccvbc commented Aug 30, 2022

njakes commented Sep 26, 2022

Daeraxa commented Sep 26, 2022

globalhuman commented Sep 27, 2022

personalizedrefrigerator commented Aug 16, 2022 •

edited

Loading

personalizedrefrigerator commented Aug 17, 2022 •

edited

Loading

laurent22 Aug 18, 2022 •

edited

Loading

personalizedrefrigerator commented Aug 18, 2022 •

edited

Loading

personalizedrefrigerator commented Aug 19, 2022 •

edited

Loading

personalizedrefrigerator commented Aug 20, 2022 •

edited

Loading

personalizedrefrigerator commented Aug 27, 2022 •

edited

Loading

personalizedrefrigerator commented Aug 29, 2022 •

edited

Loading

personalizedrefrigerator commented Aug 29, 2022 •

edited

Loading

personalizedrefrigerator commented Aug 29, 2022 •

edited

Loading

tomasz1986 commented Aug 29, 2022 •

edited

Loading