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

Added autofocus feature to input components in input.py - (09/15/24). #788

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

Conversation

StefanoCandiani
Copy link

As detailed in Issue #775 (Support Autofocus), I believe I added functionality of autofocus to each of the input functions (InputText, InputFloat, InputInt, and InputNumeric). Each of these will have a default parameter called focus that is initially set to False though the user can set to true or false. For InputText and InputNumeric, the focus variable is directly linked to the TextField autofocus default variable while for InputFloat and InputInt, the focus variable is merely passed on when the InputNumeric is called in the return statement. This is my first ever contribution so I'm not sure if I did it correctly or not so please let me know if I did a mistake or if I can optimize the code. Thank you!

Copy link
Collaborator

@iisakkirotko iisakkirotko left a comment

Choose a reason for hiding this comment

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

Hi @StefanoCandiani! Thanks a lot for the PR.

I had two comments:

  • I think the user-facing name of the attribute should be autofocus, since it more clearly communicates the intended use of the attribute and matches the HTML and Vuetify attribute.

  • We should elaborate on the use in the docstring. I'd like to see:

    • That the element is focused on page load or when it is first mounted (in a solara.lab.ConfirmationDialog, solara.lab.Menu, or inside an if statement, for example), and
    • That only one element per page (or more properly per element that gets mounted, I guess) should have autofocus=True

    mentioned.

@StefanoCandiani
Copy link
Author

Would this be fine? I changed focus to autofocus and expounded on the autofocus component in the docstrings. Thanks!

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