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

add button to toggle password visibility #3894

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

noahvogt
Copy link

Add Button to Toggle Password Visibility

Changes
Add Button in UserLoginCredentialsFragment.kt to toggle the visibility of the password when logging in. This would be a big QOL improvement for me, as I am sure it is for others that don't have a proper keyboard connected to their TV's as well.

Issues
Fixes #255

Preview
It looks like this:
show_pw
hide_pw

android:id="@+id/password"
style="@style/Input.Default"
android:layout_width="0dp"
<androidx.constraintlayout.widget.ConstraintLayout
Copy link
Member

Choose a reason for hiding this comment

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

Merge this constraint layout into the parent constraint layout. Traditional layouts are inefficient.

app:layout_constraintBottom_toBottomOf="@id/toggle_password_visibility_button"
/>

<ImageButton
Copy link
Member

Choose a reason for hiding this comment

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

The background is wrong when focused:

image

The ripple effect on activation seems to be fine.

@@ -514,6 +514,7 @@
<string name="past_week">Past week</string>
<string name="scheduled_in_next_24_hours">Scheduled in next 24 hours</string>
<string name="past_24_hours">Past 24 hours</string>
<string name="show_or_hide_password">show or hide password</string>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<string name="show_or_hide_password">show or hide password</string>
<string name="show_or_hide_password">Show password</string>

@@ -0,0 +1,5 @@
<vector xmlns:android="http://schemas.android.com/apk/res/android" android:height="24dp" android:tint="#000000" android:viewportHeight="24" android:viewportWidth="24" android:width="24dp">
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure both icons are formatted the same as all existing icons, including the same attributes for tint.

@@ -58,6 +60,10 @@ class UserLoginCredentialsFragment : Fragment() {
}
}

with (binding.togglePasswordVisibilityButton) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with (binding.togglePasswordVisibilityButton) {
with(binding.togglePasswordVisibilityButton) {

binding.togglePasswordVisibilityButton.setImageResource(R.drawable.ic_visibility_off)
}

binding.password.setSelection(binding.password.text.length)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this line does anything

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Aug 27, 2024
@nielsvanvelzen
Copy link
Member

Hey @noahvogt, are you still interested in finishing this pull request? it's currently waiting for you to address my review comments above. It's otherwise good to merge (if rebased).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Conflicts prevent merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show/hide password button for login
3 participants