Bug-1986319: Allow auto selection of full hash matches#1024
Bug-1986319: Allow auto selection of full hash matches#1024moijes12 wants to merge 2 commits intomozilla:mainfrom
Conversation
✅ Deploy Preview for mozilla-perfcompare ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1024 +/- ##
==========================================
+ Coverage 96.19% 96.22% +0.02%
==========================================
Files 111 111
Lines 3155 3178 +23
Branches 712 719 +7
==========================================
+ Hits 3035 3058 +23
Misses 118 118
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| }, []); | ||
|
|
||
| // Helper to check if the search term matches a full 40 character revision hash | ||
| const isFullRevisionHash = (searchTerm: string) => |
There was a problem hiding this comment.
Since we are permitting partial searches (by short hash), this improvement should also apply to short hashes, not solely to full revision hashes.
Note: This request improvement was added by me in bugzilla, but it is actually something that came up in our latest feedback google form. It's missing some details and it was anonymous, so we'll need to make some decisions regarding the behaviour.
@kala-moz What do you think?
There was a problem hiding this comment.
@beatrice-acasandrei I agree with your comment on the bug. The behavior should remain the same except now the checkbox is checked and the revision is shown in the component after the hash is pasted. This should also work for short hashes.
|
@moijes12 I've run out of time today so I'll review this tomorrow! |
Sure Kala. |
kala-moz
left a comment
There was a problem hiding this comment.
Thanks for taking this on! Right now this somewhat works for full hashes. The problem is that after pasting the full hash, the revision dropdown doesn't appear below the search input so the user can't change their selection. And if the user manually deletes the revision from the view, the revision dropdown still doesn't appear. The user is stuck with their original selection. We'll have to investigate why this happens. And like Beatrice said, this should also work for short hashes.
|
Thanks @kala-moz and @beatrice-acasandrei I will work on this and add the changes to the PR |
moijes12
left a comment
There was a problem hiding this comment.
Thanks @kala-moz
I have added some unit tests for the functionality to select full hash.
For short hash, is there any character length that I need to look at ? From what I see in the commit selection box, the hash has a length of 12 characters. Should I use that ?
Updated `SearchInputAndResults` to: * Track input value as state * Check if the input is a hash and if it matches an existing has * Auto select the hash if it matches the full hash
Added tests for functionality to auto select matched full hash
@moijes12 No, use the For example: |
| <Box ref={containerRef}> | ||
| <SearchInput | ||
| value={inputValue} | ||
| onFocus={() => setDisplayDropdown(true)} |
There was a problem hiding this comment.
So, we still have the problem of the results drop-down not appearing when the user clicks on the input after pasting the full hash. Also, if the user deletes the selected revision, the results drop-down also doesn't appear when the user clicks the input. Try this to fix the problem and get the full list.
onFocus={() => {
setDisplayDropdown(true);
if (!inputValue) {
void searchRecentRevisions('');
}
}}
| (rev) => rev.id === completeHashMatch.id, | ||
| ); | ||
| if (alreadySelected) { | ||
| setDisplayDropdown(false); |
There was a problem hiding this comment.
Don't set the display to false because that's not the desired behavior after a selection. So please delete this.
| return; | ||
| } | ||
| } | ||
| setDisplayDropdown(false); |
There was a problem hiding this comment.
Same here. Delete this please.
There was a problem hiding this comment.
Hi Alex, thanks for the updates. We still have to work on the behavior because we don't want to hide the drop-down nor clear the input after the hash is pasted and the revision selected. We also need to be able to get the full list of revisions after a selected revision is deleted. I've added some suggestions throughout.
| ); | ||
| if (alreadySelected) { | ||
| setDisplayDropdown(false); | ||
| setInputValue(''); |
There was a problem hiding this comment.
Don't clear the input value here either. That's also not the desired behavior
| } | ||
| } | ||
| setDisplayDropdown(false); | ||
| setInputValue(''); |
There was a problem hiding this comment.
Same here. Please delete.
|
Thanks @kala-moz This feedback is great. I will work on this and get it done |
Updated
SearchInputAndResultsto:@kala-moz @beatrice-acasandrei
Fixes Bug-1986319