Feature/entry number for display and navigation #1215

palmtreefrb · 2021-10-09T15:27:10Z

Description

This PR will allow users to enter an index number to move focus to an entry. With the below features.
Should display the current entry's index number (1-based)
Should be editable
Should protect against invalid input, including numbers out of range (revert to current entry number)
Upon user typing valid entry number, should navigate to that entry
For desktop and mobile.

Type of Change

Only keep lines below that describe this change, then delete the rest.

New feature (non-breaking change which adds functionality)
UI change

Screenshots

How Has This Been Tested?

Enter an entry index
Change filter
Change focus in list

Checklist:

I have performed a self-review of my own code
I have reviewed the title/description of this PR which will be used as the squashed PR commit message
I have commented my code, particularly in hard-to-understand areas
I have added tests that prove my fix is effective or that my feature works

megahirt

Nice work Rick! This will be a nice improvement, especially for Margarete who is having difficulty navigating her dictionary. I would like to know if the <form ngSubmit> idea is feasible. If it's not then let me know and I will approve as is. Thanks for checking that out. Let me know if you have questions.

megahirt · 2021-10-10T10:17:23Z

src/angular-app/languageforge/lexicon/editor/editor.component.ts

+  gotoToEntry(index: number, isValid: boolean) {
+    if (isValid) {
+      let id = this.editorService.getIdInFilteredList(Number(index));
+      this.editEntryAndScroll(id);
+      let gotoElement = (<HTMLInputElement>document.getElementById('goto'));
+      gotoElement.value = '';
+    }
+  }


It feels like the function name should be goToEntry() instead of gotoToEntry` Can you rename?

The if statement around validation isn't very nice, since I believe AngularJS has built-in validation as part of a form using ngSubmit. e.g. you could wrap the input in a
and I think the min / max validation will happen for free. No need to pass in the form validation state.

That's what I'm thinking in my head, however I haven't tried it out. Let me know how you get on.

There already is a function with that name line 921 goToEntry(entryId: string)

For the ngSubmit suggestion to work would require an input type of "submit" the text box is input type of "numeric"

megahirt · 2021-10-10T10:17:28Z

src/angular-app/languageforge/lexicon/editor/field/dc-entry.component.html

@@ -13,7 +13,7 @@
            <div class="comment-bubble-entry">
                <comment-bubble control="$ctrl.control" field="$ctrl.fieldName" model="$ctrl.model" parent-context-guid="$ctrl.contextGuid"></comment-bubble>
            </div>
-            &nbsp;Entry
+            &nbsp;Entry, Number: {{$ctrl.entryIndex}}


Is this addition intentional?

This was discussed in Slack, Alex liked the suggestion to add the entry number to the card.
https://sil-language-software.slack.com/archives/CGDKZ7NTV/p1633621109496300

megahirt · 2021-10-15T13:45:15Z

@palmtreefrb I'm sorry to keep you waiting on this one. Your PR work and the discussion we had at standup last week revealed that this feature was not as well defined as I thought. Here are my revised UX requirements. I think you almost have it.

1. The entry number (1-based) is always visible in the input box. Rationale: the entry number shows the user the entry's position in the dictionary. It may shift as entries are added or deleted, but it should be a reliable, easy to remember way to get back to that place in the dictionary. We should not need to show the entry number anywhere else because it should always be visible here.
2. The entry number is editable when there are no search filters in place. If a filter is active, the entry number input and the back/forward buttons are greyed out. Rationale: back/forward and entry number navigation are useful for browsing the dictionary as a whole. They are not useful in navigating a results subset.
3. An invalid or out-of-range input results in a red box showing an invalid state (thanks @palmtreefrb !).
4. If the input loses focus (valid or invalid), the input reverts back to the current entry's index number. Rationale: As per number 1 above the input always shows the current entry's index number. Losing focus should restore the entry number to what the user sees as the current entry
5. Pressing Enter changes the current entry
6. A tooltip should say something like "Enter a number and press Enter to go to that entry"

rmunn · 2021-11-04T05:33:53Z

I was testing this feature today, to approve it, and found a bug. Point 2 of #1215 (comment) says:

The entry number is editable when there are no search filters in place. If a filter is active, the entry number input and the back/forward buttons are greyed out. Rationale: back/forward and entry number navigation are useful for browsing the dictionary as a whole. They are not useful in navigating a results subset.

If the entry number input is edited and you press Enter, while there is no filter in place, this works correctly. Typing in 5 and pressing Enter jumped me to entry number 5, as it should.

However, if there is a filter in place, then typing in the entry number input and pressing Enter does not behave correctly. I set a filter that showed only two items, then I typed 5 into the input and pressed Enter.

Expected result: I get jumped to entry number 5 (the fifth entry in the unfiltered list).
Actual result: I get taken out of the editor view and into the index view, with the filter in place so I only see two items.

This is good work, and I'm looking forward to getting it merged in once all the bugs are fixed.

rmunn · 2021-11-05T03:26:33Z

Looked at it again, and I missed the part of the design where it says that the entry number input should also be grayed out along with the Previous/Next buttons. So forget what I said about expected behavior when typing into the entry number input with a filter in place. It's actually supposed to be disabled in that circumstance. So the bug report is just "entry number input wasn't disabled when Previous/Next buttons were disabled". This was in Firefox on Linux, in case that matters.

megahirt · 2021-11-07T05:33:01Z

@palmtreefrb I've been out of action for several weeks and I think once you address @rmunn 's bug, and he tests it, then we can merge this. Looks like a rebase is in order too.

palmtreefrb · 2021-11-09T00:36:00Z

@palmtreefrb I've been out of action for several weeks and I think once you address @rmunn 's bug, and he tests it, then we can merge this. Looks like a rebase is in order too.

The bug is fixed and I have rebased this branch with develop.

rmunn · 2021-11-09T09:47:42Z

I pulled the latest version of the branch and built it, and I can still type into the entry number input when there's an active filter. It's not actually disabled yet.

Also, I've found a bug: when you type a search expression into the filter, the entry number can change, without any apparent cause. For example, here's what happens when there is no filter and I have the sixth item selected:

Now I type ab into the filter, and the following happens:

It seems very unintuitive to have the entry number change from 6 to 8 when I added a filter expression. (After I typed the letter a the entry number was still 6. It only became 8 when I typed the b). With different filter expressions, sometimes the entry number stayed 6, once it became 7, and once it became 15. So the results can vary, but there's definitely a bug somewhere.

Actually, the bug might not even be in your code, @palmtreefrb. The logic around the filtered list view is complex enough that we could actually have some other bugs in there, that your code is merely exposing but not causing. But before we can merge this PR, we certainly do need to find and fix those bugs, because if the user sees the entry number jump around it will be very confusing.

rmunn · 2021-11-09T09:59:44Z

Found another behavior I'm not entirely happy with. When you type in the filter list view, the entry number input becomes red once it's out of range. E.g. you're viewing entry number 6, but you've filtered the list view so it only shows 2 items. The entry input box shows 6 and has a red border.

I don't believe we should show a red border around the entry input field when it's disabled. The red border (validation failed) should only be displayed when the input is enabled and incorrect. So if the field is being correctly disabled when a filter is applied, then there's no need to change the validation logic. You'll just need to have the red border show up when the field is enabled AND the validation has failed.

I think I would add a couple more requirements to the list that Chris made above:

An invalid or out-of-range input box shows a red border if and only if the field is editable. (So if there's a filter and the field has become uneditable, the red border will never display).
The entry number only changes when one of the following occurs:
- The user clicks the Previous or Next buttons.
- The user types in the entry number box and presses Enter.
- The user clicks on a different item in the list. (See below for more about this one)

In particular, the entry number should not change when filtering items. It will end up showing a number that's outside the range, but if I was looking at entry number 6 in the unfiltered list, filtering the list should not tell me I'm looking at entry number 8.

Now for the "clicks on a different item in the list" part. In my testing, I filtered the list by typing ab and then clicked on one of the list items (the first one in the filtered list). When I clicked on it, the entry list number showed "1". Then I removed the fitler (by clicking the X button next to the filter search text) and the entry number jumped to 10. I know why that's happening: when I clicked on the entry in the filtered list, it was the 1st item (index 0) in the filtered list but the 10th (index 9) of the whole dictionary. And so the existing code that's supposed to look up an item's position in the list is using the filtered list to look it up. But what we should be using is the unfiltered list here.

In other words, there's another new requirement coming out of my testing:

The entry index number always shows the index of that entry in the complete, unfiltered dictionary.

This might be a new requirement that you weren't aware of, in which case I apologize for the extra work that it will take to meet this requirement. But I'm pretty sure this is always how we intended the feature to work.

rmunn · 2021-11-10T09:20:47Z

Found one reason for the entry input not being disabled. You've created a filterIsOn function to detect whether the filter is active, but its logic is only checking the option list and not the filter text. So when I type something into the filter text input, filterIsOn() returns false when it should return true.

Fortunately, the fix is simple. There's already a filterActive() method on the this.entryListModifiers object. So all you need to do is call entryListModifiers.filterActive() instead of filterIsOn() and you'll have the right logic already, instead of duplicating that logic somewhere else.

In other words, this should fix that part of the issue.

diff --git a/src/angular-app/languageforge/lexicon/editor/editor.component.ts b/src/angular-app/languageforge/lexicon/editor/editor.component.ts
index 597dc7b65..d8b2dcacc 100644
--- a/src/angular-app/languageforge/lexicon/editor/editor.component.ts
+++ b/src/angular-app/languageforge/lexicon/editor/editor.component.ts
@@ -317,8 +317,7 @@ export class LexiconEditorController implements angular.IController {
 
   filterIsOn() {
     const mod = this.entryListModifiers;
-    let on = mod.filterBy != null && mod.filterBy.option !=null;
-    return on;
+    return mod.filterActive();
   }
 
   shouldShowFilterReset() {

I've tested this and it does disable the entry number input correctly. The input still shows red when it's disabled, and it shouldn't, but that's a different issue I don't have time to track down right now.

palmtreefrb · 2021-11-10T15:53:18Z

diff --git a/src/angular-app/languageforge/lexicon/editor/editor.component.ts b/src/angular-app/languageforge/lexicon/editor/editor.component.ts
index 597dc7b65..d8b2dcacc 100644
--- a/src/angular-app/languageforge/lexicon/editor/editor.component.ts
+++ b/src/angular-app/languageforge/lexicon/editor/editor.component.ts
@@ -317,8 +317,7 @@ export class LexiconEditorController implements angular.IController {
 
   filterIsOn() {
     const mod = this.entryListModifiers;
-    let on = mod.filterBy != null && mod.filterBy.option !=null;
-    return on;
+    return mod.filterActive();
   }
 
   shouldShowFilterReset() {

Robin, this was by design.The requirement was to disable the index search when a filter was applied. Shouldn't we allow a search if only sorting the list?

rmunn · 2021-11-11T02:53:43Z

Robin, this was by design.The requirement was to disable the index search when a filter was applied. Shouldn't we allow a search if only sorting the list?

The filterActive() method is defined like this:

  filterActive = () => !!(this.filterText() || this.filterBy && this.filterBy.option);

That won't consider the sort at all, but it will consider the filterText, which the current code is not considering.

Sorting is an issue I hadn't considered. Do we want the entry number to change when the user reverses the sort order, or not? The way the code is right now, if I click on entry 1 of my 30-entry project and reverse the sort order, the entry number field becomes 30. If I click on entry 2 and reverse the sort order, it becomes 29. My gut feeling is this is not what users are expecting, and that we should make the entry number be "the entry's order in the normally-sorted order of the dictionary", not the current sort selection. But I could see an argument either way for it, and that's something we can discuss at our next standup meeting.

At the moment, though, the filterIsOn function should be using filterActive so that it takes filterText into consideration as well. And the entry number needs to not be based on the filtered list, but on the whole, unfiltered list. We do not want the behavior where I click on the first item of the filtered list and see "entry number 1", then unfliter the list and the entry number jumps to 10. It should have shown "10" even though that entry was the first item in the filtered list, because that's the entry number in the whole dictionary.

rmunn · 2021-11-11T09:52:30Z

After further review of the LF code, it seems that some of the issues with the entry number jumping around are difficult to solve and aren't something that this PR introduces. For example, sorting entries actually changes the this.entries array and not just this.visibleEntries. (Look at the sortEntries function in editor-data.service.ts). So it's going to be difficult to implement this feature as originally envisioned, because we're going to have to make some fundamental changes to how we handle the entry filter and sort feature if we want the entry number to look stable.

palmtreefrb · 2021-11-11T12:59:21Z

After further review of the LF code, it seems that some of the issues with the entry number jumping around are difficult to solve and aren't something that this PR introduces. For example, sorting entries actually changes the this.entries array and not just this.visibleEntries. (Look at the sortEntries function in editor-data.service.ts). So it's going to be difficult to implement this feature as originally envisioned, because we're going to have to make some fundamental changes to how we handle the entry filter and sort feature if we want the entry number to look stable.

I had come to this very same conclusion after diving deeper into the workings of the filter and sort implementation code and wanted to discus on the next stand-up.

…ion ng-disabled to back, forward and goto elements

…y entry-index="$ctrl.entryIndex() removed on rebase, adding back

This change adds a ng-blur handler that resets the value of the numeric input to the current entry index. This fixes the input having a sticky invalid state/red box when the user clicks a button or loses focus on the input. Additionally, we hide the controls when the filter is active, instead of disabling.

megahirt · 2022-01-11T09:05:49Z

This latest commit fixes the misbehaving red box behavior on blur and hides the control completely when the filter is active, thereby avoiding the wonky behavior discussed earlier in this PR.

Here is a demo of the control working after this latest commit.

@palmtreefrb and @longrunningprocess wanted to get your review on this before merging.

megahirt · 2022-01-11T09:25:18Z

@josephmyers and I paired on this PR today and also had a bit of a design discussion on moving the entry index navigation closer to the entry list UI, which is captured in #1274

palmtreefrb linked an issue Oct 9, 2021 that may be closed by this pull request

entry number for display and navigation #1013

Closed

5 tasks

megahirt requested changes Oct 10, 2021

View reviewed changes

palmtreefrb force-pushed the feature/entry-number-for-display-and-navigation branch from 2078a23 to e401994 Compare October 25, 2021 17:45

palmtreefrb requested a review from megahirt October 26, 2021 17:20

palmtreefrb added 9 commits January 10, 2022 23:49

initial commit for goto entry

ba450de

Add entry number to Entry card

b8962e8

Remove unneeded import

f54e82d

clear goto after valid move

9ebcca2

Add function to check if filtering and sorting is on. Add implementat…

66dc626

…ion ng-disabled to back, forward and goto elements

Added controller model to imnput element to track current enrty index

eb275ee

Change input tool tip message

4f6e938

Remove unused binding to ui element

527f373

Min and Max validation properties were inadvertently removed. dc-entr…

2f3e08f

…y entry-index="$ctrl.entryIndex() removed on rebase, adding back

megahirt self-assigned this Jan 11, 2022

megahirt force-pushed the feature/entry-number-for-display-and-navigation branch from e223e20 to 2f3e08f Compare January 11, 2022 06:53

megahirt approved these changes Jan 11, 2022

View reviewed changes

josephmyers mentioned this pull request Jan 11, 2022

feat: Move Entry Index Navigation and Total Entries Closer #1274

Closed

palmtreefrb merged commit c51ccf2 into develop Jan 11, 2022

palmtreefrb deleted the feature/entry-number-for-display-and-navigation branch January 11, 2022 22:40

megahirt added the enhancement label Jan 13, 2022

alex-larkin mentioned this pull request Jan 21, 2022

bug: Button bar too tall on mobile #1290

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/entry number for display and navigation #1215

Feature/entry number for display and navigation #1215

palmtreefrb commented Oct 9, 2021

megahirt left a comment •

edited

Loading

megahirt Oct 10, 2021

palmtreefrb Oct 10, 2021 •

edited

Loading

palmtreefrb Oct 10, 2021

megahirt Oct 10, 2021

palmtreefrb Oct 10, 2021 •

edited

Loading

megahirt commented Oct 15, 2021 •

edited by palmtreefrb

Loading

rmunn commented Nov 4, 2021

rmunn commented Nov 5, 2021

megahirt commented Nov 7, 2021

palmtreefrb commented Nov 9, 2021

rmunn commented Nov 9, 2021

rmunn commented Nov 9, 2021

rmunn commented Nov 10, 2021

palmtreefrb commented Nov 10, 2021 •

edited

Loading

rmunn commented Nov 11, 2021

rmunn commented Nov 11, 2021

palmtreefrb commented Nov 11, 2021

megahirt commented Jan 11, 2022

megahirt commented Jan 11, 2022

Feature/entry number for display and navigation #1215

Feature/entry number for display and navigation #1215

Conversation

palmtreefrb commented Oct 9, 2021

Description

Type of Change

Screenshots

How Has This Been Tested?

Checklist:

megahirt left a comment • edited Loading

Choose a reason for hiding this comment

megahirt Oct 10, 2021

Choose a reason for hiding this comment

palmtreefrb Oct 10, 2021 • edited Loading

Choose a reason for hiding this comment

palmtreefrb Oct 10, 2021

Choose a reason for hiding this comment

megahirt Oct 10, 2021

Choose a reason for hiding this comment

palmtreefrb Oct 10, 2021 • edited Loading

Choose a reason for hiding this comment

megahirt commented Oct 15, 2021 • edited by palmtreefrb Loading

rmunn commented Nov 4, 2021

rmunn commented Nov 5, 2021

megahirt commented Nov 7, 2021

palmtreefrb commented Nov 9, 2021

rmunn commented Nov 9, 2021

rmunn commented Nov 9, 2021

rmunn commented Nov 10, 2021

palmtreefrb commented Nov 10, 2021 • edited Loading

rmunn commented Nov 11, 2021

rmunn commented Nov 11, 2021

palmtreefrb commented Nov 11, 2021

megahirt commented Jan 11, 2022

megahirt commented Jan 11, 2022

megahirt left a comment •

edited

Loading

palmtreefrb Oct 10, 2021 •

edited

Loading

palmtreefrb Oct 10, 2021 •

edited

Loading

megahirt commented Oct 15, 2021 •

edited by palmtreefrb

Loading

palmtreefrb commented Nov 10, 2021 •

edited

Loading