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

feat: #15237 toggle password visibility on shared albums #15238

Merged

Conversation

imakida
Copy link
Contributor

@imakida imakida commented Jan 10, 2025

No description provided.

@imakida imakida changed the title feat: toggle password visibility on shared albums feat: #15237 toggle password visibility on shared albums Jan 10, 2025
Copy link
Member

@bo0tzz bo0tzz left a comment

Choose a reason for hiding this comment

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

@imakida imakida requested a review from bo0tzz January 11, 2025 03:32
@imakida
Copy link
Contributor Author

imakida commented Jan 11, 2025

@bo0tzz Thanks for reviewing my PR. I made the changes you requested.

I did add some e2e tests, are they enough?

Comment on lines 84 to 85
<form class="flex" novalidate autocomplete="off" {onsubmit}>
<div class="relative mr-2">
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 you need the changes here either, right? Get rid of the class="flex" on the form and remove the div again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The class="flex" on the form is to place the password input and the submit button on the same row per the original design. The PasswordField component is a div so it pushes the button into the next row if the form is not a flexbox.

It looks like I can get rid of <div class="relative mr-2"> that wraps the password field and move the margin to onto the button <Button class="ml-2" ...>.

Copy link
Member

Choose a reason for hiding this comment

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

I know what that flex is. I just didn't see how swapping the input with the password component would make the difference here. If it's required though we obv can keep it, thus the question :)
Instead of ml/mr I'd rather work with gap I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. That makes sense. Thanks for the input. I removed the margins and set the column gap.

Copy link
Member

@bo0tzz bo0tzz left a comment

Choose a reason for hiding this comment

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

ty!

@alextran1502 alextran1502 merged commit a39fbcb into immich-app:main Jan 12, 2025
33 checks passed
yosit pushed a commit to yosit/immich that referenced this pull request Jan 13, 2025
…mmich-app#15238)

* feat: toggle password visibility on shared albums

* feat: toggle password visibility on shared albums

* use password-field component

* remove div wrapping PasswordField

---------

Co-authored-by: Ian <[email protected]>
arctic-foxtato pushed a commit to arctic-foxtato/immich that referenced this pull request Jan 14, 2025
…mmich-app#15238)

* feat: toggle password visibility on shared albums

* feat: toggle password visibility on shared albums

* use password-field component

* remove div wrapping PasswordField

---------

Co-authored-by: Ian <[email protected]>
vladd11 pushed a commit to vladd11/immich that referenced this pull request Jan 25, 2025
…mmich-app#15238)

* feat: toggle password visibility on shared albums

* feat: toggle password visibility on shared albums

* use password-field component

* remove div wrapping PasswordField

---------

Co-authored-by: Ian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants