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

Save and restore state of focus #35

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

uzurpastor
Copy link

Hi Andrey,

Sorry for the delay.

I've implemented what was described in the last comment of the PR #33 discussion : #33 (comment)

Current PR also is related to issue #32.

Screencast:

Screencast.from.2024-08-23.23-13-36.mp4

@uzurpastor uzurpastor marked this pull request as ready for review August 23, 2024 20:16
package.json Outdated
@@ -38,20 +38,20 @@
}
],
"devDependencies": {
"@logux/eslint-config": "^53.2.1",
Copy link
Owner

Choose a reason for hiding this comment

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

Revert this changes since they break CI (also each PR should be focused on a single thing)

Copy link
Author

Choose a reason for hiding this comment

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

Done. I apologize for the changes added to the commit due to inattention

@ai
Copy link
Owner

ai commented Aug 26, 2024

We need to fix CI: fix tests and update size-limit (to track size changes of every commit)

@uzurpastor
Copy link
Author

Hi. Sorry for my disappearance for a month

Notes and questions related to the test update:

  1. I changed the size-limit parameter on my local
  2. I faced a blocker when updating tests. I don't understand how to debug and then understand how to modify the jump.test.ts file. This was the reason why I quit this job. I tried:
    1. using console.log() to see some output in the console, but it returns the string HTMLAnchorElement {} and I don't know how to check these objects in the output.
    2. use debugger, but it is skipped by the executor
  3. I do not understand the command c8 pnpm bnt and why it works.

@ai
Copy link
Owner

ai commented Sep 19, 2024

I changed the size-limit parameter on my local

Maybe you forgot to push it?

There are no changes in PR https://github.com/ai/keyux/pull/35/files

Or what is the question?

I don't understand how to debug and then understand how to modify the jump.test.ts file

You need to add new test(). In this test you will create a DOM of some page.

Then you need to emulate user interactions with DOM API.

using console.log() to see some output in the console, but it returns the string HTMLAnchorElement {} and I don't know how to check these objects in the output.

console.log(node.innerHTML)

use debugger, but it is skipped by the executor

Maybe it is because of c8. Try to run just:

pnpm bnt

or even:

node --test --import tsx --enable-source-maps

I do not understand the command c8 pnpm bnt and why it works.

pnpm c8 bnt

  1. bnt is better-node-test, just node --test (built-in test runner in Node.js) with a few twicks like out-of-the-box TS support and nicer output.
  2. c8 CMD is a coverage check. It runs CMD command (bnt in our case with unit tests) and check what lines was executed in JS/TS files. With this tool we are sure that we run every line in our tests.
  3. pnpm CMD just call binary from node_modules

Feel free to ask extra question if you want to go deeper (or if I didn’t understand you correctly).

@uzurpastor
Copy link
Author

Hi. Done! Waiting for your review!

@@ -53,14 +71,21 @@ export function jumpKeyUX() {
}, 50)
}

function restoreFocus(event) {
event.preventDefault()
lastFocusedElement.focus({ focusVisible: true })
Copy link
Owner

Choose a reason for hiding this comment

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

What if it was deleted from DOM?

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