Mobile: Add markdown toolbar #6703
Conversation
Math blocks and regions of inline math were missing their original containing nodes.
|
Can you also add screenshots in landscape mode? |
| describe('Matching', () => { | ||
| describe('should match start and end of bolded regions', () => { | ||
| const spec: RegionSpec = RegionSpec.of({ | ||
| template: '**', | ||
| }); | ||
|
|
||
| it('should return the length of the match', () => { |
There was a problem hiding this comment.
Because of variables shared within logical groups of tests, I decided to use nested describes here. I've heard that nested/multiple describe blocks can make it difficult to run a specific test (I use a VSCode extension that doesn't have trouble with this, though...).
How might I reorganize this?
There was a problem hiding this comment.
You could split the tests into two files, which would have the added advantage of being able to run these tests concurrently. See for instance how it's done for the synchonizer tests - synchronizer.basic.test.ts, synchronizer.e2ee.test.ts, etc.
There was a problem hiding this comment.
I've broken markdownCommands.test.ts into multiple files. I'm thinking of each describe in markdownReformatter as a test with sub-tests, however. As each is testing a different RegionSpec with findInlineMatch, I think it makes more sense to either
- Turn each
describeinto a single test with multipleexpects - Leave the nested
describes
| /** | ||
| * @jest-environment jsdom | ||
| */ |
There was a problem hiding this comment.
With
// @jest-environment jsdom
JSDom was not used for this test suite.
(a document is required by EditorView).
| export interface RegionSpec { | ||
| // The name of the node corresponding to the region in the syntax tree | ||
| nodeName?: string; | ||
|
|
||
| // Text to be inserted before and after the region when toggling. | ||
| template: { start: string; end: string }; | ||
|
|
||
| // How to identify the region | ||
| matcher: RegionMatchSpec; | ||
| } | ||
|
|
||
| export namespace RegionSpec { // eslint-disable-line no-redeclare | ||
| interface RegionSpecConfig { | ||
| nodeName?: string; | ||
| template: string | { start: string; end: string }; | ||
| matcher?: RegionMatchSpec; | ||
| } | ||
|
|
||
| // Creates a new RegionSpec, given a simplified set of options. | ||
| // If [config.template] is a string, it is used as both the starting and ending | ||
| // templates. | ||
| // Similarly, if [config.matcher] is not given, a matcher is created based on | ||
| // [config.template]. | ||
| export const of = (config: RegionSpecConfig): RegionSpec => { |
There was a problem hiding this comment.
The purpose of namespace RegionSpec is a set of constructor functions for the RegionSpec interface.
I think I find this easier to read than the class version of RegionSpec in an earlier version of the code, however, of has a several optional/default parameters (which I would like to avoid). How might I structure this differently?
Note that the toolbar is partially pushed offscreen in the second screenshot — this ensures that there is enough space to edit the note. It would, of course, be better to shrink the buttons, or hide other parts of the UI.... |
|
Just so you know there some linter errors: https://github.com/laurent22/joplin/runs/7577564001?check_suite_focus=true#step:10:4515 |
|
When the editor opens, the toolbar looks like this:
Is that expected? because I thought it was broken initially. Also do we want these submenus in the toolbar? For example shouldn't frequently used buttons be immediately accessible? How about on tablet where there's plenty of space? I feel we should start with a thorough comparison of other mobile editor apps. Like Apple note, Samsung note, Evernote, etc. I suspect there's a better way to do this. Also please use |
Yes. That is intentional. My intention was to group related functionality (actions that might be frequently used together) into different submenus to keep the distance between related actions small. (Additionally, this design seemed easier to make accessible to screen readers).
I would prefer to avoid changing the order of buttons -- I personally find it annoying if actions move around (unless the actions are search suggestions/based on what I'm typing). To make frequently used buttons immediately accessible, I would prefer to allow the user to customize the toolbar in settings. This, however, could be left to a follow-up PR.
It might make sense to stack submenus below parent menus in this case. This would make the toolbar somewhat similar to the ribbon interface in MS Office. |
|
Apple Notes has a similar-ish design on my iPhone: IMG_0837.MOV |
|
Thanks for checking. Samsung Note is also doing something similar so let's stay on this idea then. I'd prefer if we change the icons though because they're a bit hard to decipher. The "Aa" for text formatting of Apple seems pretty good, and then another button that generally represent lists. By the way, could you provide the complete list of formatting buttons you have at this point, and the category you have them under? |
|
And that's how Samsung Note looks. When pressing on the highlighted button, that popup appears. It needs be explicitly dismissed by clicking on the cross.
|
|
Closing until #6707 is merged and the toolbar has been redesigned. |
|
I want to share my opinion
Screenrecording_20220830_101334.mp4Screenrecording_20220830_103521.mp4 |
Summary
Screenshots