-
-
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 markdown toolbar #6753
Mobile: Add markdown toolbar #6753
Conversation
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. |
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. |
); | ||
} | ||
|
||
if (!props.visible) { |
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 put that at the top of the function? That way you skip the loop if the component is not visible.
const buttonSize = 56; | ||
|
||
// Displays a list of buttons with an overflow menu. | ||
const Toolbar = (props: ToolbarProps) => { |
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.
There's too many of these sub-components. Could you create a MarkdownToolbar folder and move these components to file please?
// 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(() => { |
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.
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.
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.
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.
editorControl.searchControl.hideSearch(); | ||
} else { | ||
editorControl.searchControl.showSearch(); | ||
} |
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 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.
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've updated this with a searchStateRef = RefObject<SearchState>
. As such, neither searchControl
nor editorControl
need to be re-created when SearchState
changes.
I've checked the UI and it looks pretty good. Just a few observations:
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. |
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...
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].
It looks like the CI errors are related to the desktop app:
https://github.com/laurent22/joplin/runs/7918363808?check_suite_focus=true#step:6:4436 |
…d/replaced with something else
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. |
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/explanationThe 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). |
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'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
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. |
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? |
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've removed it on Android! |
…' into pr/markdownToolbar2
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. |
Brilliant, thanks guys for checking this. Let's wait till CI is done and we can merge. |
I want to share my opinion
Screenrecording_20220830_103521.mp4Screenrecording_20220830_101334.mp4 |
Will there be a way to create check boxes with this toolbar? |
Yes, its in the lists section |
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? |
Summary
Screenshots