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 key binding to copy code #691

Merged
merged 2 commits into from
Jul 6, 2021
Merged

add key binding to copy code #691

merged 2 commits into from
Jul 6, 2021

Conversation

Sneezry
Copy link
Member

@Sneezry Sneezry commented Jun 24, 2021

Press arrow keys to select a code, press enter to copy.

Animation

@Sneezry Sneezry requested a review from mymindstorm June 24, 2021 19:10
@codecov
Copy link

codecov bot commented Jun 24, 2021

Codecov Report

Merging #691 (3df1282) into dev (3b3a86b) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #691      +/-   ##
==========================================
- Coverage   15.09%   15.07%   -0.03%     
==========================================
  Files          15       15              
  Lines        1457     1459       +2     
  Branches      320      320              
==========================================
  Hits          220      220              
- Misses       1219     1221       +2     
  Partials       18       18              
Impacted Files Coverage Δ
src/store/Accounts.ts 9.78% <0.00%> (-0.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b3a86b...3df1282. Read the comment docs.

return entry === firstEntry ? 0 : -1;
},
findNextEntryIndex(
entries: OTPEntry[],
Copy link
Member

Choose a reason for hiding this comment

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

On all functions, entries, shouldFilter, and filter are all computed properties and don't need to be passed as arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@mymindstorm
Copy link
Member

I really like this!

Bugs / Other:

  • When entering password, it does not function. Need to hit tab or reopen the extension for it to work.
  • If I click anywhere else (even an entry) it will not let me use arrow keys until tab or reopen.
    • Should we listen for keys on the document?
    • Should we focus entries on click?

@Sneezry
Copy link
Member Author

Sneezry commented Jul 5, 2021

When entering password, it does not function. Need to hit tab or reopen the extension for it to work.

Fixed. After applying password, the first entry will be auto focused.

If I click anywhere else (even an entry) it will not let me use arrow keys until tab or reopen.

Entry will have focus when it is clicked. If you click anywhere else, the entry should not have focus. And is as designed.

Should we listen for keys on the document?

I don't think so. By following w3 accessibility guide, tab should be used to select active element, and arrow keys should be used to select items in an active group element. If the group element is not active, we should not response to the action.

Should we focus entries on click?

Yes. This is a bug in previous commit. I have fixed that in the recent commit.

Copy link
Member

@mymindstorm mymindstorm left a comment

Choose a reason for hiding this comment

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

Looks great

@@ -14,6 +14,7 @@
v-model="searchText"
v-bind:placeholder="i18n.search"
type="text"
tabindex="-1"
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be able to be tabbed to, but it is ok to not allow it until we get around to overhaling keyboard nav for the header and menu. Hopefully compoentizing everything will help at the second version of that.

@mymindstorm
Copy link
Member

Will this supersede #689?

@Sneezry
Copy link
Member Author

Sneezry commented Jul 6, 2021

Will this supersede #689?

I suppose. Number keys could be hard to use to copy a specific entry. Imaging how to copy the 7th entry in the list. I have to count entries from top to bottom.

@Sneezry Sneezry merged commit af907ee into dev Jul 6, 2021
@Sneezry Sneezry deleted the key-binding branch July 6, 2021 04:40
@Sneezry Sneezry mentioned this pull request Aug 23, 2021
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.

2 participants