Enhance SecretTag UI with button-based toggle #21
Conversation
umutdz
commented
Mar 27, 2025
umutdz
commented
- Create MySecretInput class using horizontal layout for better UI
- Add Show/Hide toggle button on the right side similar to PathTag's Browse button
- Implement keyboard shortcut (Ctrl+T) for toggling password visibility
- Maintain consistent styling with other form elements
- Improve user experience for password input fields
- Removed unused imports
- Create MySecretInput class using horizontal layout for better UI - Add Show/Hide toggle button on the right side similar to PathTag's Browse button - Implement keyboard shortcut (Ctrl+T) for toggling password visibility - Maintain consistent styling with other form elements - Improve user experience for password input fields
| yield Label(desc) | ||
| yield Label("") | ||
| self.focusable_.clear() | ||
| self.focusable_.extend(w for w in self.widgets if isinstance(w, (Input, Changeable))) | ||
|
|
||
| def on_mount(self): | ||
| self.widgets[self.focused_i].focus() | ||
| if self.widgets and 0 <= self.focused_i < len(self.widgets): |
There was a problem hiding this comment.
In what use case there is focused_i set and the widget cannot be focusable?
There was a problem hiding this comment.
The code safely handles all these cases by first validating the index, then checking if it's a composite widget with a focusable input, before falling back to direct widget focusing.
| if self.widgets and 0 <= self.focused_i < len(self.widgets): | ||
| widget = self.widgets[self.focused_i] | ||
| # If it's a widget with an input field (like FilePickerInput), focus that input | ||
| if hasattr(widget, "input") and hasattr(widget.input, "focus"): |
There was a problem hiding this comment.
I'd prefer you add the method focus to the widget it is missing than checking the hasattr here :)
There was a problem hiding this comment.
It's definitely the cleaner approach, though it would require refactoring several widget classes. I'll implement this
|
|
||
| BINDINGS = [Binding("ctrl+t", "toggle_visibility", "Toggle visibility")] | ||
|
|
||
| def __init__(self, tag: SecretTag, *args, **kwargs): | ||
| DEFAULT_CSS = """ |
There was a problem hiding this comment.
Are you able to move the binding ctrl+t somewhere to a method so that we may let the programmer to set it dynamically in the future? (That's another feature, you don't have to elaborate this if you dont want to.)
There was a problem hiding this comment.
I checked the docs. Textual doesn't support modifying the bindings at runtime, but they have some solutions with dynamic actions, but I think this is a very different feature
| # Eğer arbitrary bir buton ise, pozisyonunu ayarla | ||
| if isinstance(self._arbitrary, Button): | ||
| self._arbitrary.styles.dock = "right" | ||
| def focus(self): |
There was a problem hiding this comment.
This is better but I beg for even better approach: It seems, the self.input has only file picker and my secret. Let this two inherit from:
class ChangeableWithInput(Changeable):
def __init__(self):
common things
def on_input_submitted(self, event: Input.Submitted) -> None:
"""Handle Enter key in the input field to submit the form."""
self._link.facet.adaptor.app.action_confirm()
def focus(self):
self.input.focus()
Then, let it every other widget accept the tag as the parameter too. That was your good idea. Change textual_adaptor/widgetize to stop setting o._link = tag and instead pass it as the first arg to all the widgets.
elif tag._is_a_callable():
o = MyButton(tag, tag.name)
and changeable accepts the tag in the init
class Changeable:
def __init__(self, tag, *args, **kwargs):
self._link = tag # rename self._link to tag
super().__init__(*args, **kwargs)
If that's too much work, just confirm the self.input is used by these two classes only and I'll do the rest.
There was a problem hiding this comment.
well, I had to rewrite some parts. This is definitely a better approach, but I hope I understand your request correctly, please check it
| @@ -111,6 +116,7 @@ class MySecretInput(Horizontal, Changeable): | |||
| def __init__(self, tag: SecretTag, **kwargs): | |||
| super().__init__() | |||
| self.tag = tag | |||
| self._link = tag | |||
There was a problem hiding this comment.
Already set in widgetize (but my idea was the Changeable.init will set this for every widget, I've written it alongside)
|
Minor bug, This was again an excellent work and thanks a lot!! :) |
|
Tell me class ChangeableWithInput(Changeable):
"""Base class for widgets that contain an input element"""
def __init__(self, tag: Tag, *args, **kwargs):
super().__init__(tag, *args, **kwargs)
def on_input_submitted(self, event: Input.Submitted) -> None:
"""Handle Enter key in the input field to submit the form."""
self.tag.facet.adaptor.app.action_confirm()
def focus(self):
"""Focus the input element of this widget."""
super().focus()-> shouldnt there be self.input.focus() ? |
Oh, that's a missed one. Thanks a lot :) |
good catch! I was testing, just forgot it |
