Skip to content
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

Add missing decorators in note list previews #1814

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Jan 4, 2020

When showing each item in the note list we run the content decorators on them
in order to show checkboxes instead of the markdown syntax for a checkbox and
we also highlight the matching text from the search box.

Previously we have only been showing the first match for each decorator because
of a couple small bugs:

  • the task prefix matcher wasn't using the m multi-line flag in its RegExp
    pattern
  • the search filter text was only replacing the first occurrance, see JS
    "replaceAll()" proposal

In this patch we're rewritten decorateWith to automatically create a global
RegExp pattern for the search text and the task prefix RegExp pattern has been
corrected.

After this patch you can now see all of the decorated items in the note list;
for example, if you had two checklist items in a note then you can now see
multiple checkboxes in the note list whereas before you'd only see the first
one and then the second one would appear as - [ ] or - [x]

Testing

Open an account with notes having multiple repeated text in the first few lines.

For example…(one appears in each line)

# A short poem
One river,
Two stones,
In the zone.

Search for the repeated pattern, such as one

You should find that in develop you'll only see the first result highlighted
but in this branch all of the ones should be highlighted.

Repeat the experiment with a checklist…

# Groceries
 - [ ] Eggs
 - [ ] Chicken
 - [ ] Milk

In develop only the first item will render as a checklist but in this branch
all three should, if they are visible in the preview.

Verify that if you search for regular-expression-y patterns then you'll only
get results matching the literal text and not the pattern itself.

Patterns I use:

4 a number - /\d/

If you search for \d then the \d in the note should be highlighted, not the 4

Before

missing-decorators mov

After

all-the-decorators mov

@dmsnell dmsnell requested a review from a team January 4, 2020 08:31
Copy link
Contributor

@belcherj belcherj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great.

When showing each item in the note list we run the content decorators on them
in order to show checkboxes instead of the markdown syntax for a checkbox and
we also highlight the matching text from the search box.

Previously we have only been showing the first match for each decorator because
of a couple small bugs:
 - the task prefix matcher wasn't using the `m` multi-line flag in its RegExp
   pattern
 - the search filter text was only replacing the first occurrance, see JS
   "replaceAll()" proposal

In this patch we're rewritten `decorateWith` to automatically create a global
RegExp pattern for the search text and the task prefix RegExp pattern has been
corrected.

After this patch you can now see all of the decorated items in the note list;
for example, if you had two checklist items in a note then you can now see
multiple checkboxes in the note list whereas before you'd only see the first
one and then the second one would appear as ` - [ ]` or `- [x]`
@dmsnell dmsnell force-pushed the fix/note-list-missing-decorators branch from a496888 to 7da052e Compare January 7, 2020 00:06
@dmsnell
Copy link
Member Author

dmsnell commented Jan 7, 2020

Rebased to catch renames from #1813 in develop

@@ -3,6 +3,6 @@ export const selectors = {
markdownRoot: '[data-markdown-root]',
};

export const taskPrefixRegex = /^(\s*)(-[ \t]+\[[xX\s]?\])/g;
export const taskPrefixRegex = /^(\s*)(-[ \t]+\[[xX\s]?\])/gm;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mirka can you remember if there was any specific reason not to include all search results here by adding the m? I didn't see any mention of it in the commit log but I don't want to accidentally introduce a bug you intentionally avoided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like between the time I added this regex and now, the behavior of the Comfy/Expanded note list views have changed. Before, it showed the title and only the first line of the body, with Expanded showing more of the first line if it was a long paragraph. Now, it's concatenating multiple lines of the body if the lines are short.

So back then we didn't need the m flag, simply because a note body preview would never show more than one task item line. (In that sense, it's actually the g flag that was superfluous!)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! that makes sense. we did update the preview generation so that it would show the start of the first few lines of the note instead of collapsing them. we missed the impact here then when we made that change.

thanks for chiming in and helping @mirka!

@dmsnell dmsnell merged commit 2a131c5 into develop Jan 7, 2020
@dmsnell dmsnell deleted the fix/note-list-missing-decorators branch January 7, 2020 18:09
@codebykat codebykat added this to the 1.14 milestone Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants