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

Fixes autofocus on password field when Ledger Live Desktop is password protected #537

Merged

Conversation

tomav
Copy link
Contributor

@tomav tomav commented Jun 30, 2022

📝 Description

Fixes autofocus on password field when Ledger Live Desktop is password protected.
The code to do this exists, but is target ID lockscreen-password-input is missing in the view.

  useEffect(() => {
    let subscribed = false;
    const onKeyDown = () => {
      const input = document.getElementById("lockscreen-password-input");
      if (input) {
        input.focus();
      }
    };
    if (isLocked) {
      subscribed = true;
      window.addEventListener("keydown", onKeyDown);
    }
    return () => {
      if (subscribed) {
        window.removeEventListener("keydown", onKeyDown);
      }
    };
  }, [isLocked]);

❓ Context

LIVE-3129

✅ Checklist

  • Test coverage
  • Atomic delivery
  • No breaking changes

📸 Demo

https://youtu.be/pw8xn9YJhXk

🚀 Expectations to reach

Please make sure you follow these Important Steps.

Pull Requests must pass the CI and be internally validated in order to be merged.

@changeset-bot
Copy link

changeset-bot bot commented Jun 30, 2022

🦋 Changeset detected

Latest commit: a59590b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
ledger-live-desktop Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jun 30, 2022

@tomav is attempting to deploy a commit to the LedgerHQ Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Jun 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
ledger-live-github-bot ✅ Ready (Inspect) Visit Preview Jul 28, 2022 at 1:37PM (UTC)
live-common-tools ✅ Ready (Inspect) Visit Preview Jul 28, 2022 at 1:37PM (UTC)
native-ui-storybook ✅ Ready (Inspect) Visit Preview Jul 28, 2022 at 1:37PM (UTC)
react-ui-storybook ✅ Ready (Inspect) Visit Preview Jul 28, 2022 at 1:37PM (UTC)

@github-actions github-actions bot added the desktop Has changes in LLD label Jun 30, 2022
@codecov
Copy link

codecov bot commented Jun 30, 2022

Codecov Report

Merging #537 (cbb1435) into develop (134355d) will not change coverage.
The diff coverage is n/a.

❗ Current head cbb1435 differs from pull request most recent head a59590b. Consider uploading reports for the commit a59590b to get more accurate results

@@           Coverage Diff            @@
##           develop     #537   +/-   ##
========================================
  Coverage    47.44%   47.44%           
========================================
  Files          620      620           
  Lines        27752    27752           
  Branches      7130     7130           
========================================
  Hits         13166    13166           
  Misses       13462    13462           
  Partials      1124     1124           
Flag Coverage Δ
test 47.44% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 134355d...a59590b. Read the comment docs.

@Justkant
Copy link
Contributor

We could use a ref to make this more resilient to changes in the codebase instead of relying on an id: https://reactjs.org/docs/refs-and-the-dom.html#refs-and-function-components

Example for a functional component:

function CustomTextInput(props) {
  // textInput must be declared here so the ref can refer to it
  const textInput = useRef(null);
  
  function handleClick() {
    textInput.current.focus();
  }

  return (
    <div>
      <input
        type="text"
        ref={textInput} />
      <input
        type="button"
        value="Focus the text input"
        onClick={handleClick}
      />
    </div>
  );
}

Copy link
Contributor

@Justkant Justkant left a comment

Choose a reason for hiding this comment

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

LGTM

@tomav tomav changed the title Fixes autofocus on password field when Ledgr Live Desktop is password protected Fixes autofocus on password field when Ledger Live Desktop is password protected Jun 30, 2022
@gre
Copy link
Contributor

gre commented Jul 1, 2022

added a changelog

@elbywan elbywan added the fork Pull request base branch comes from a fork. label Jul 13, 2022
@ae-ledger
Copy link
Member

Hi @tomav,

Could you have a look on the conflicted files and resolve it ? In this case apps/ledger-live-desktop/src/renderer/components/IsUnlocked.js

@gre gre self-assigned this Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop Has changes in LLD fork Pull request base branch comes from a fork.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants