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

Autofocus input field after submitting new item #2310

Merged
merged 3 commits into from
Aug 25, 2022
Merged

Autofocus input field after submitting new item #2310

merged 3 commits into from
Aug 25, 2022

Conversation

jwu910
Copy link
Contributor

@jwu910 jwu910 commented Aug 24, 2022

Thank you for your contribution to the Pi-hole Community!

Please read the comments below to help us consider your Pull Request.

We are all volunteers and completing the process outlined will help us review your commits quicker.

Please make sure you

  1. Base your code and PRs against the repositories developmental branch.
  2. Sign Off all commits as we enforce the DCO for all contributions

  • What does this PR aim to accomplish?:

This provides one minor enhancement to the whitelist/blacklist form to allow users to more quickly add more domains quickly.

  • How does this PR accomplish the above?:

This PR utilizes jquery's .focus() method on the domainEl in the groups-domains code. Since the inputs for both the exact and regex fields end up being the same domainEl, this works for both tabs Domain and RegexFilter.

2022-08-23 21 21 20

  • What documentation changes (if any) are needed to support this PR?:

No foreseeable documentation changes required.


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

  • I have read the above and my PR is ready for review. Check this box to confirm

@yubiuser
Copy link
Member

Thanks for your PR. I think we should make it consistent across the group pages, so could you add this to adlists and groups as well? Clients could be tricky or not desired as it uses a drop down menu.

@jwu910
Copy link
Contributor Author

jwu910 commented Aug 24, 2022

Sure thing! Will take a look tomorrow. To confirm group pages are the "group-" prefixed js files?

I haven't looked at the clients page yet, but can see what inputs are available and see if it would make sense to make any changes there, too.

@jwu910
Copy link
Contributor Author

jwu910 commented Aug 24, 2022

@yubiuser, adding focus to the select might indeed feel kinda clunky if the dropdown pops open right as you submit a new group-client. In the future, maybe if this was converted to an input with autocomplete with the available options this might be more straight forward since all of the options wouldn't need to show unless the user starts typing.

Latest commit adds the same functionality to the groups and groups-client pages.

Thanks!

@yubiuser
Copy link
Member

Thanks. Sorry, for coming up with just another idea: should we extend the autofocus to all pages with input fields? Hope I did not forget one: Query log, Local DNS, CNAME Records, Pi-hole diagnosis, Search Adlists, Network.

@jwu910
Copy link
Contributor Author

jwu910 commented Aug 24, 2022

I think the inputs with the action to add to a list makes sense, the search inputs from what I've seen update the lists as the user is typing so focus hasn't been lost yet.

Seems like local dns and cname make sense. Would just updating these two in addition make sense?

@yubiuser
Copy link
Member

Your're right, the search field keep the focus, but it might be nice to have them autofocus right from the start (page load)?

@jwu910
Copy link
Contributor Author

jwu910 commented Aug 24, 2022

Your're right, the search field keep the focus, but it might be nice to have them autofocus right from the start (page load)?

The search bars seem to be the first item in the tab index already, even though this is a fairly small application, would (probably have to force some slower load times) wonder if a user could be interacting with another part of the page and focus be forced back to the input once the page has completed loading.

Might be an edge case, but idk how slow this can get. my local testings seems to be pretty quick. but i just installed pi-hole 2 days ago and assume my lists are pretty small

@rdwebdesign
Copy link
Member

I think auto-focus for search box in queryads.php (Search Adlists) is a good idea.

On the other side, I don't think we need this for every small search like the one in queries.php (Query Log), network.php, etc.

@jwu910
Copy link
Contributor Author

jwu910 commented Aug 24, 2022

If that's what the team wants, i can add that change in as well. Just wanted to provide my insight, too. will check back in a little later

@rdwebdesign
Copy link
Member

First of all: No need to change it if you fell it's a bad idea.

If that's what the team wants, i can add that change in as well.

Maybe I wasn't clear in my comment:
I think ONLY the queryads.php (Search Adlists) would be useful (but not mandatory).

In my personal opinion, adding auto-focus to every search fields would be disruptive, at least in some cases.

@jwu910
Copy link
Contributor Author

jwu910 commented Aug 24, 2022

@rdwebdesign that makes sense, this is my first time contributing to this project so I was just deferring to others who seem to have been contributing regularly.

I'd propose possibly creating a separate ticket to either discuss or work on the search inputs

jwu910 added 3 commits August 24, 2022 16:50
Signed-off-by: Joshua Wu <Joshua.wu.0910@gmail.com>
Signed-off-by: Joshua Wu <Joshua.wu.0910@gmail.com>
Signed-off-by: Joshua Wu <Joshua.wu.0910@gmail.com>
@rdwebdesign rdwebdesign requested a review from yubiuser August 25, 2022 02:56
@yubiuser
Copy link
Member

I'm fine with how it is now. I only made some suggestions, no need to implement those. Let me know if you think something should be added, if not this can be merged. Feel free to open a new PR for the other discussed pages.

Thanks for your contribution.

@jwu910
Copy link
Contributor Author

jwu910 commented Aug 25, 2022

No problem, interested to see what else i may be able to contribute to. Just got pi-hole set up this week, so as I interact with the UI, i may open a few issues. I'll file an issue for the auto focus on queryads for now. that way someone else might be able to pick it up if they find time first.

I don't have anything else to commit at this time thanks

@jwu910
Copy link
Contributor Author

jwu910 commented Aug 25, 2022

Actually looks like I'm not involved on the discourse site enough to submit a feature request. I can leave that up to the members here if they feel it's needed

@yubiuser yubiuser changed the title Autofocus domain element after submitting new whitelist domain Autofocus input field after submitting new item Aug 25, 2022
@yubiuser yubiuser merged commit 31bfeb5 into pi-hole:devel Aug 25, 2022
@jwu910 jwu910 deleted the autofocus-domain-whitelist branch August 25, 2022 16:11
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.

3 participants