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(input): implement password type to input #381

Merged
merged 28 commits into from
Feb 7, 2023
Merged

feat(input): implement password type to input #381

merged 28 commits into from
Feb 7, 2023

Conversation

AykutSarac
Copy link
Member

Added password type for bl-input, added toggle action for text visibility.
Additionally, closes #302

@muratcorlu
Copy link
Contributor

Thanks for the contribution @AykutSarac ! Please update tests to cover new functionality.

@AykutSarac
Copy link
Member Author

@muratcorlu Done! Let me know if anything is missing.

@muratcorlu
Copy link
Contributor

Apparently, Microsoft Edge has its own password reveal button by default. We may consider hiding it for the consistency https://learn.microsoft.com/en-us/microsoft-edge/web-platform/password-reveal

Copy link
Contributor

@muratcorlu muratcorlu left a comment

Choose a reason for hiding this comment

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

I also realized that, current implementation can have issues with the buttons placed by password managers (like icloud keychain or 1password) To avoid this, I think we need to put icons next to the input instead of putting it on top of input with absolute positioning. But this will need a bigger refactor on the component because currently we use internal input for the borders of the component. Probably we'll need to wrap internal input with another element and handle positioning icons in it.

muratcorlu and others added 3 commits January 24, 2023 15:34
* disabled style fixed
* small size added
* vertical alignments fixed
* internal component structure refactored for creating extra flexibility
for upcoming features (like trailing/leading text), better reliability and
maintainability
@muratcorlu
Copy link
Contributor

@AykutSarac I opened a PR (#389) that proposes some refactor on Input component. Those changes can help about some stuff in this PR (like avoiding covering password manager buttons with password hide/show buttons). Please have a look at it. Please feel free to reach me with any questions.

AykutSarac and others added 2 commits February 3, 2023 14:05
Co-authored-by: Murat Çorlu <127687+muratcorlu@users.noreply.github.com>
Signed-off-by: Aykut Saraç <aykutsarac0@gmail.com>
@AykutSarac
Copy link
Member Author

Updated accordingly, fyi @muratcorlu.

Copy link
Contributor

@muratcorlu muratcorlu left a comment

Choose a reason for hiding this comment

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

We have a good neat work here. Congrats @AykutSarac

@buseselvi
Copy link
Contributor

buseselvi commented Feb 7, 2023

Great work! We can use neutral kind icon button to open and close the eye like in the design (2nd image) in every input state (default, focused, typing, entered and error).

This is because when we use the danger button for the eye in case of error it looks dangerous to press but it is not. @AykutSarac @muratcorlu

1 image
2 image

@muratcorlu
Copy link
Contributor

Oh, good catch @buseselvi is it ok to use natural color in every case? Including focus state.

@muratcorlu muratcorlu merged commit 0be574b into Trendyol:next Feb 7, 2023
@github-actions
Copy link

github-actions bot commented Feb 7, 2023

🎉 This PR is included in version 2.0.0-beta.66 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Apr 6, 2023

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Input's name property not present on TypeScript
3 participants