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

Svelte Tag Editor v2 #1264

Merged
merged 160 commits into from Sep 15, 2021
Merged

Svelte Tag Editor v2 #1264

merged 160 commits into from Sep 15, 2021

Conversation

hgiesel
Copy link
Contributor

@hgiesel hgiesel commented Jun 29, 2021

Take 2. Closes #1079.
Disclaimer: This is not supposed to go with 2.1.45. So be free to ignore it, while .45 is not out yet.

Screenshot 2021-06-29 at 13 13 25

Notes:

  1. I tried to emulate the visuals of the tag editor on iOS, and the functionality of the tag editor on Desktop. Two ways you can see this is using arrows keys, to navigate between the tags, and using backspace (or delete) to delete beyond tags.

  2. WithAutocomplete is a very general component, that could also be reused in a wide variety of scenarios. I actually thought it could be useful, when we implement the NoteType / Deck selector in Svelte, and we don't go with a modal.

  3. The Tag badge (same icon as in browser) is clickable, and replaces exposes the "Focus Tags" shortcut. Clicking the badge behaves slightly different from the shortcut. Clicking the badge will always focus the new tag input, whereas the shortcut toggles it.

    • On a small screen, when we set the buttons nowrap, we could also set the tags nowrap, and make the "AddTag" icon sticky, that way users could still easy add new tags, even thought the end of the tags is beyond the viewport.
  4. The autocomplete currently has one randomly generated item. This shows you when the suggestions would update.

  5. Instead of Ctrl+Tab, just Tab will navigate the menu. Ctrl+Tab is reserved on macOS (App switcher).

    1. This allows for two alternative ways to navigate the autocomplete. First is Tab + Shift-Tab + Enter. However Up + Down + Right works just as well. Selecting an autocompletion item will move the caret to the end of the tag, and pressing right will open a new tag to the right (if the tag is the last one).
  6. The autocomplete navigates bottom to top when it is a dropup (most relevant item will be at the bottom).

  7. Entering a duplicate will highlight the old one and discard the new one.

Todos:

  • Finalize a Space behavior
  • Deal with cases when the autocomplete happens to be a dropdown (row instead of row-reverse) (Autocomplete for the tag editor will always be a dropup now)
  • Connect tag suggestions API endpoint in Rust. Any pointers here are welcome.
  • Implement nowrap version.
  • Collapsing tags behavior
  • Copy-paste behavior
  • Clicking on tag suggestions

@hgiesel
Copy link
Contributor Author

hgiesel commented Jun 29, 2021

(UPDATE: this is resolved) Another possible issue I just noticed:

Screenshot 2021-06-29 at 16 57 13

The webview doesn't stretch.

@dae
Copy link
Member

dae commented Jun 30, 2021

Looking forward to this!

I gave it a brief try, but got a JS error when I tried to select one of the completions, so I presume that's still a work in progress.
One other issue I noticed is that really long tags cause the viewport to expand to the right, requiring scrolling.

Once 2.1.45's out the door and any release issues dealt with, I can help with the Rust side of things if required. Some sort of weighting system for the search results would be nice, as we briefly discussed in the past.

@hgiesel
Copy link
Contributor Author

hgiesel commented Jun 30, 2021

Actually rebasing caused the JS errors to occur (probably Bootstrap 5.0.2), and it has to do with the WithDropdownMenu. A related bug actually also affect the deckoptions menu: When you set New cards to 21, then open up the dropdown menu for reversion, and then set the new cards back to 20 by using the spinners, the dropdown menu will stay open, and you can't close it. I'm looking into it.

@dae
Copy link
Member

dae commented Jul 1, 2021

Have only given it a brief play so far, but seems to work well after that DropDown fix :-)

@kleinerpirat
Copy link
Contributor

kleinerpirat commented Jul 3, 2021

This is such a cool PR!

Suggestion

Extra long (hierarchical) tags feel a bit out of place with this new look. Short tags (like marked, leech, WIP, etc.) and hierarchical tags seem like two separate concepts that deserve separate treatment. I wonder if there is a neat way to remedy this. Perhaps you could make hierarchical tags collapse, only show the last node of the hierarchy with a visual indication that it is hierarchical?

If that is too radical of an idea, I would suggest to make the text cut off to the left, like it did previously, since the last nodes of a hierarchy contain the info that is most relevant to the current note.

image
image

@hgiesel
Copy link
Contributor Author

hgiesel commented Jul 3, 2021

I like the collapsing idea. An issue is that each row that the tags create, takes away from the space that is left to navigate the fields. The following is a note from the Anking deck with quite a lot of tags:

Screenshot 2021-07-03 at 12 41 39

Idea 1: This what looks like if the tags were collapsed to their last segment:

Screenshot 2021-07-03 at 12 41 52

Two instead of six rows. We could show the full tag on hover, which would still make it very discoverable.

Idea 2: However we could also (even in addition) collapse the tags themselves, and show an icon (v, ^) at the end, which at click will uncollapse / collapse the tags.

image

On mobile, if we make the tags nowrap, and allow for vertical scrolling maybe the problem does not even arise?

@kleinerpirat
Copy link
Contributor

Idea 1 looks very promising to me. It removes a lot of redundant information and I believe everyone will intuitively understand the meaning of ... at the beginning of the tags.

We could show the full tag on hover, which would still make it very discoverable.

Perhaps a full reveal on click (in edit mode) would already be enough? I fear that hovering/expanding long tags could cause a major displacement of other tags, which could get frustrating if it happens on accident.

@hgiesel
Copy link
Contributor Author

hgiesel commented Jul 3, 2021

Perhaps a full reveal on click (in edit mode) would already be enough? I fear that hovering/expanding long tags could cause a major displacement of other tags, which could get frustrating if it happens on accident.

I meant, revealed as a tooltip! I forgot to mention that :)

@dae
Copy link
Member

dae commented Jul 4, 2021

Idea 1 looks great. If full is shown on hover + edit, presumably that'll make most people happy?

@dae
Copy link
Member

dae commented Jul 31, 2021

(We'll likely need a few hotfix releases after 2.1.45, so I'd like to hold off on merging non-trivial changes for a week or two)

@dae
Copy link
Member

dae commented Sep 2, 2021

AnkiWeb stuff has dragged on a bit, but hopefully after this weekend I'll be able to look into the Rust part of this. Looking forward to playing with it with real data :-)

One thing I noticed - with the old editor, hitting enter will complete the top match, but it now accepts the tag as typed. It appears you can accomplish similar by pressing tab instead, though the completions move from the bottom rather than the top, which feels a bit strange - was that deliberate?

@hgiesel
Copy link
Contributor Author

hgiesel commented Sep 3, 2021

It was deliberate, yes. My reasoning was the following:

  • The old tag suggestions were always a dropdown, however this one will have to be a dropup because the dropdown could not exceed beyond the bottom edge of the window
  • When you get the list of suggestions, they will be of unknown size, so it's not clear where the uppermost suggestion will lie on the screen. This makes it hard to navigate to with your eyes
  • The suggestion at the bottom of the dropup however is the one closest to the tag input, which is why it's the easiest to navigate to with your eyes

That's why I decided to reverse the order you tab through suggestions. However I'm open to suggestions. :)

P.S.
Something I spent some time implementing but haven't mentioned anywhere (I think) is that you can Ctrl-Click and Shift-Click tags.
For example, to clear all tags of a note, you can Ctrl-Click one tag, then click A while still holding Ctrl, and press Delete.
Shift-clicking will select ranges (just like in the browser sidebar).

@dae
Copy link
Member

dae commented Sep 6, 2021

The old Qt dropdown will only show about 6 matches at once. Couldn't we do the same thing with the dropup? That would mean the first match would always appear in a predictable place. It wouldn't be directly next to the input box, but if the top entry is highlighted, the eye should be able to jump to it fairly easily. I appreciate what you're trying to achieve by having the matches start next to the input area, but I have a feeling users will find the inverted navigation a bit difficult to get used to. WDYT?

ctrl/shift click features sound handy!

@dae
Copy link
Member

dae commented Sep 8, 2021

I've pushed the Rust end of things into this branch. It wasn't clear to me how I should be grabbing the candidate string for completion, so I've left that part for you. What's the goal of the unicode component separator by the way?

@dae
Copy link
Member

dae commented Sep 8, 2021

Sorry, had a typo - have force-pushed a fix.

@hgiesel
Copy link
Contributor Author

hgiesel commented Sep 9, 2021

After some more thorough testing, I have accumulated this list of remaining issues:

  • Scroll to suggestion when tabbing through Autocomplete
  • Long suggestions in Autocomplete display weird
  • Dropdown in deck-options is broken (regression)
  • (possibly) Make Bottom bar sticky rather than fixed I will try this again when we move the AddCards/EditCurrent to a single webview window

EDIT: Playing around with the Anking deck, if there around 4.7k~ suggestions it causes a lag of around 2 seconds.
However I wonder where we should do the cutoff, and how many we should display at the same time.

Currently I display 6 suggestions at a time (the container being scrollable). Maybe we could display a few more suggestions at a time (8~12), but make the cutoff for fetching suggestions at a lower number, so we avoid a lag when typing at any cost.

I really don't know which one is the more typical use case:

  1. Type to narrow it down to just a few results
  2. Scroll down and find the correct tag among hundreds/thousands other tags.

Maybe @AnKingMed could help out here.

@AnKingMed
Copy link

@hgiesel how can I help? Most of my tags are very long and there are ~10-15 per card but I'm not sure what you need from me to assist on this

@hgiesel
Copy link
Contributor Author

hgiesel commented Sep 9, 2021

@AnKingMed
Here you can see me editing the tags on one of the Anking notes:

image

What I did was enter #AK_Step1_v7:: and then scroll till that entry. #AK_Step1_v7:: gave me around 4.7k suggestions in the Anking deck. However, fetching all 4.7k suggestions from the database and rendering them causes some lag while you type. This is where the trade-off comes in:

  1. We could provide fewer suggestions in the dropdown (~100 - 500), which will make typing guaranteed lag-free, however which will cut off the suggestions at some point (which are sorted alphabetically), which means the tag you're looking for might not be included in the list, because it starts with a letter later in the alphabet.

  2. We could provide more suggestions (~1200 - 1500+) if they're available, which will cause some felt lag when you type (maybe half a second), but will make it more likely that the tag you're looking for is in the suggestions.

So, which one do you think is more important? Make typing lag-free or provide more tag suggestions? Or, how long could you accept the lag to be, if it means you get more suggestions?

@AnKingMed
Copy link

I see what you mean now. At the moment #AK_Step1_v7:: is not a tag anyone would ever enter. They would be more likely to enter one of the sub tags or sub-sub tags and search through those, which are far more limited. I also do not think people are really adding a lot of new cards to that deck with those tags, mostly just adding tags to cards that are already there. So the browser is going to be the place where this is an issue, not the "add card" dialog. If you're only limiting to 500 or so, I think that would probably work just fine for my purposes

@dae
Copy link
Member

dae commented Sep 12, 2021

This is looking great Henrik, and almost ready for merging I think. A couple of issues I noticed:

  • When a tag is selected from the completion that causes the tag list to wrap, it results in the completion area moving. This can be quite distracting when the user is scrolling through a list of tags of varying lengths:
wrap.mp4
  • If the user has a tag 'foo::bar', typing 'foo' shows it, then 'foo:' and 'foo::' show nothing, but it appears again when you type 'foo::b'. Could we make it work as the user has typed the separator too? And maybe we should also consider allowing a single : instead of requiring the user to type ::?
  • The tooltip causes the document to expand, and as you move the cursor you can end up in a situation where the cursor/page is constantly redrawing (the video has not captured it, but the cursor at the end was changing back and forth rapidly)
hover.mp4

+ convert from u32 in backend method
@dae
Copy link
Member

dae commented Sep 12, 2021

Just pushed a very minor change to the Rust code. One other thing I noticed - currently with focus on Front, pressing tab once jumps to Back, but pressing Tab again does not move to the tag input area. Would it be easy to fix that?

@hgiesel
Copy link
Contributor Author

hgiesel commented Sep 15, 2021

When a tag is selected from the completion that causes the tag list to wrap, it results in the completion area moving. This can be quite distracting when the user is scrolling through a list of tags of varying lengths

How would you suggest to change this behavior? The jQuery tag editor shows the same behavior.

One thing I could imagine is that clicking the AddTag icon / using Ctrl+Shift+T shortcut, puts the TagInput before the first tag (closer to the icon), which would prevent the completion area from shifting, only the tags that follow it.
Screenshot 2021-09-15 at 01 42 02

And maybe we should also consider allowing a single : instead of requiring the user to type ::?

I've been wary about that because there might be users with colons in the tag hierarchy, like tag:sub, or want to enter colons for other private reasons. I'd really like to get user feedback on the space behavior, too. I have to say I got used to it really quick.

Just pushed a very minor change to the Rust code. One other thing I noticed - currently with focus on Front, pressing tab once jumps to Back, but pressing Tab again does not move to the tag input area.

I know this is a change to before, but I thought it makes most sense for several reasons:

  • Tab now is used for tag suggestions in the TagEditor. So it might be confusing if a user enters something, expecting to tab through the autocomplete items, however there are no autocomplete items, and suddenly Tab moves him out of the tag editor.
  • There is Ctrl+Shift+T, which focuses the tag editor more reliably (and can also unfocus it).
  • Another reason: I prepared a PR akin to Tag Selector for after this PR, which would shift the tag input in the AddCards window heavily from "quickly typing in tags" to "toggling on the tags you want".

@dae
Copy link
Member

dae commented Sep 15, 2021

How would you suggest to change this behavior?

Would making the dropup be the width of the entire input area + anchored to the top of the input area make sense/be practical to implement? No strong feelings here, just throwing out ideas.

One thing I could imagine is that clicking the AddTag icon / using Ctrl+Shift+T shortcut, puts the TagInput before the first tag (closer to the icon), which would prevent the completion area from shifting, only the tags that follow it.

I think like the reversed tab order change, this might be a bit surprising to users who are used to new things being added on the right.

I've been wary about that because there might be users with colons in the tag hierarchy

Fair enough - it's already a lot better now that the completions don't disappear.

Tab now is used for tag suggestions in the TagEditor. So it might be confusing if a user enters something, expecting to tab through the autocomplete items, however there are no autocomplete items, and suddenly Tab moves him out of the tag editor.

Yep, good point. Happy to run with it for now; let's see what users think. Thanks for all your work on this Henrik!

@dae dae merged commit 9daf037 into ankitects:main Sep 15, 2021
@hgiesel
Copy link
Contributor Author

hgiesel commented Sep 15, 2021

FYI this PR introduced a regression with the ImageEditor (because I introduced a bug into WithDropdown), which I've already fixed in #1324

Would making the dropup be the width of the entire input area + anchored to the top of the input area make sense/be practical to implement? No strong feelings here, just throwing out ideas.

This would probably mean that all tags have to share one dropdown. I previously had issues with this, but I could give it a try. This would give the suggestions a fixed place which would indeed be nice.

@dae
Copy link
Member

dae commented Sep 16, 2021

Might be worth a try, as it would also match the old behaviour. But if it proves to be difficult to implement, no need to spend too long on it.

@hgiesel hgiesel deleted the tageditor2 branch September 16, 2021 12:11
@dae
Copy link
Member

dae commented Mar 7, 2022

One other issue I noticed is that really long tags cause the viewport to expand to the right, requiring scrolling.

This issue seems to be back again - it seems to happen when you press up/down to select a tag that scrolls off the screen.

@hgiesel
Copy link
Contributor Author

hgiesel commented Mar 8, 2022

I'll give this a look now.

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.

None yet

4 participants