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

Enabled auto-focus on magnet url input. #450

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hotrods20
Copy link

Added JavaScript function "forceFocus" and calls to function to InputMultiple.svelte.

Added JavaScript function "forceFocus" and calls to function to InputMultiple.svelte.
@johman10
Copy link
Owner

johman10 commented Jun 8, 2022

I think it would be better to make inputMultiple and Input component pass the autofocus to the input instead. That would be easier code and it would allow us to manage the input that requires the focus from the higher component rather then from this component.

I think for the inputMultiple that we pass "autofocus" to the input with index 0?

What do you think? Would that work?

Either way we would have to have some kind of way to manage whether it should autofocus because there could be multiple inputMultiple components rendered at the same time, and if that happens this won't really work.

@hotrods20
Copy link
Author

I'll be 100% real, I have no idea how Svelte works and this is my first experience with it. I did this because I wanted it to focus on the input fields automatically but I think I understand what you're saying and I'll give it a try. I think what you are suggesting would work. I'll try some different things and update the pull request if I make a change.

@johman10
Copy link
Owner

johman10 commented Jun 8, 2022

I'll be 100% real, I have no idea how Svelte works and this is my first experience with it. I did this because I wanted it to focus on the input fields automatically but I think I understand what you're saying and I'll give it a try. I think what you are suggesting would work. I'll try some different things and update the pull request if I make a change.

No worries! Super happy so see your contribution! My main target with my comment is not to do the focus via JS but rather use the default HTML feature for this behavior.

Feel free to give it another shot when you have time. No rush. If you need help/support just let me know. :D

@hotrods20
Copy link
Author

hotrods20 commented Jun 8, 2022

Thank you and I did figure out how to pass a variable down from Add.svelte all the way to Input.svelte that would enable autofocus without JS functions, but there is a new problem that I am stuck on and is actually why I did a almost pure-JS solution previously. When you open the "Add Torrents" window, it will not focus on the text input. I am going to guess it has something to do with when the input is added to the DOM and when autofocus would actually focus on said input.

If I had to guess, I would say it's an issue of the element not being rendered but technically "accessible."

EDIT: Because of loading-initial class, the form has its visibility set to hidden which means it won't focus initially. Actually stumped here because autofocus will not work on a hidden element.

EDIT2: Fixed it in latest commit. Auto-focus works without a JS function to manually do it.

@hotrods20
Copy link
Author

Sorry, I forgot to post a reply after the last commit. The latest commit achieves the autofocus goal.

@johman10
Copy link
Owner

Hi!
Yeah, I noticed. I was playing around with this myself to better understand...

I think this solution is fine, the only things are:

  1. We don't really have to add the new prop to the Input and InputMultiple components since they will defer the restProps forward without any changes, so that minimizes the changes in this PR. :)
  2. The visibility: hidden instead of opacity: 0 or display: none was done on purpose to make the modal already size to the correct size while loading. With the current changes the modal will kind of "pop" whenever the loading state resolves.

I would like to have a look at this myself a little more before we ship it. Just to better understand the alternatives. But thanks a lot for the contribution at least, this is a great addition and perhaps something we should do in more places than just the Add modal. :)

@hotrods20
Copy link
Author

I updated my code to reflect your first point. Completely agree, my code was a bit unnecessary as I did not understand the restProps variable. Thank you for making me aware of that.

I am not completely sure about this "pop" you're noticing on the loading when using opacity: 0 instead of visibility: hidden. I will play with it a bit more to see if there is a more elegant solution. The only issue I see is from selecting the initial input element when the menu is opened. I have some ideas on how to use opacity while keeping it smooth by using the inbuilt transition property for CSS.

I would also like to note that I am testing with Firefox 101 and Chrome 102.

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