-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat(elements): add file picker #3404
base: master
Are you sure you want to change the base?
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit c130092. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
9d75f77
to
ad661bd
Compare
It seems like we cannot update the screenshots anymore due to Nx Cloud:
|
You still can. Nx cloud is IMO not very friendly for open-source projects. Everything still works the same. You can update screenshots by running the workflow action on your branch. You might have to bring your branch up to date with master first |
de0f8ae
to
8bdd687
Compare
BundleMon (elements)Files updated (3)
Total files change +27.56KB +4.46% Final result: ✅ View report in BundleMon website ➡️ |
packages/elements/src/components/file-picker/file-picker.component.ts
Outdated
Show resolved
Hide resolved
packages/elements/src/components/file-picker/file-picker.component.ts
Outdated
Show resolved
Hide resolved
packages/elements/src/components/file-picker/file-picker.component.ts
Outdated
Show resolved
Hide resolved
#handleKeyDown(event: KeyboardEvent) { | ||
this.#currentPressedKeys.add(event.key); | ||
|
||
if (this.#currentPressedKeys.has('Control') && this.#currentPressedKeys.has('k')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand the need for the currentPressedKeys
here. The KeyboardEvent already has a e.ctrlKey
, e.metaKey
and e.key
with which you have all you need, I think.
For the content of the search bar, it can be cleaner by using the input element value instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the content of the search bar, it can be cleaner by using the input element value instead
By using a ref?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
event.target.value
should also work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, but we are doing that already, no?
#handleSearch(event: KeyboardEvent) {
if (!this.openPicker) {
return;
}
if (event.key === 'ArrowDown' || event.key === 'ArrowUp' || event.key === 'Tab') {
return;
}
this.#filter((event.target as HTMLInputElement).value);
this.fileIndex = 0;
}
packages/elements/src/components/file-picker/file-picker.component.ts
Outdated
Show resolved
Hide resolved
document.body.style.overflow = 'hidden'; | ||
} | ||
|
||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this setTimeout needed because you are waiting for a re-render? I think it should be possible without and to let lit figure out the rendering from the state. Either by some state attributes or other matters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this:
this.updateComplete.then(() => this.#focusInput());
} | ||
|
||
#closePicker() { | ||
document.body.style.overflow = 'auto'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than setting style directly I think it would be cleaner to have a state attribute of the mode, and render based on the value of the state. If you are mutating style directly you run the risk of forgetting to clean them up properly and having a "drift" as your component is being used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, but I'm not sure what the implementation would look like for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
closePicker
could set a state attribute this.mode = 'closed';
. And based on the mode you could set tailwind classes. Similar to the drawer component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But how would I set tailwind classes on the document element inside this component like that?
Sorry I am confusion 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait. It's on document.body. You are confusion, but I am no paying attention
}); | ||
|
||
async function openPicker() { | ||
document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Control' })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very low-level way of emulating user input. You can use the vitest API to more closely resemble actual user input in the browser: https://vitest.dev/guide/browser/interactivity-api.html
5724665
to
18937df
Compare
Adds a file picker to the report, which allows for easier keyboard traversing over the report.
related #3370
ToDo: