silverbulletmd / silverbullet

The knowledge tinkerer's notebook
https://silverbullet.md
MIT License
2.34k stars 169 forks source link

Improve modals #840

Closed daniel-michel closed 5 months ago

daniel-michel commented 5 months ago

I have noticed some small issues with the picker and other modals that I fixed / still try to fix:

Currently the modals also have a slightly darkened background, because that comes by default with the dialog element, but I can change that back if wanted.

Here is a comparison of how things look: Before After
Screenshot_20240409_185009 Screenshot_20240409_184709
Screenshot_20240409_185047 Screenshot_20240409_184747
Screenshot_20240409_185126 Screenshot_20240409_184246
Screenshot_20240409_185205 Screenshot_20240409_184326
Screenshot_20240409_185226 Screenshot_20240409_184542

I am open to changing things, if wanted. I also don't have any prior experience with react/preact.

zefhemel commented 5 months ago

I really appreciate this, thanks! Let me know when it's ready to go.

daniel-michel commented 5 months ago

I am almost done now.

I striked through one of the points in the original post because I think it would make more sense to take a look at it separately. The part with the delegation is no longer happening but keyboard shortcuts can still be used like Ctrl+b which is strange behavior, but some other shortcuts like Ctrl+Shift+t should maybe still work when, for example, the page picker is open to quickly switch to the template picker?

And for the last other point that is still open in my original post I am not sure how to proceed: The issue that focus sticks to the text field when pressing tab. I figured out that it is caused here however I am not sure why this was done. I could just get rid of it if there are no other situations where it is required? I also noticed that while the line I linked to is run in both, firefox and chrome, it only causes the focus to actually stick to the text field in chrome, in firefox it does not appear to have any effect. Edit: I think I know the reason, which is that the editor is not correctly focused in chrome and this is fixed by Fix reference to button and mini editor focus in chrome .

daniel-michel commented 5 months ago

I noticed that my previous commit Prevent key events from propagating outside of modals prevented navigating through the results with the arrow keys. I change the mini editor to add the events directly to the div although I am again not sure why it was this way. I hope this is good?

zefhemel commented 5 months ago

Thanks. I'll have a look at this over the next few days.

zefhemel commented 5 months ago

Overall this looks good, but I found one issue (so far): when you use PgDown a few times to go down in a longer filtered list, the selection moves beyond the viewport and you can't seem to get out of it. Previously this worked. Not sure you can see this screen recording (you can't see the keyboard shortcuts unfortunately):

https://github.com/silverbulletmd/silverbullet/assets/59073/dd450b3a-2488-4cf1-9455-90e1845c55d7

Tested this in Chrome on macOS

daniel-michel commented 5 months ago

I noticed that the index clamping when using page down was the wrong way around, but that seems to have been already wrong before. That fixed it for me using Chrome and Firefox on Linux.

I have no clue why the test action on GitHub failed, running deno task test locally does not give me any errors.

zefhemel commented 5 months ago

Tests failed because dependencies went out of date. I think this looks good, let me merge it and we'll fix any bugs from there. Thanks for this!