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 password show/hide toggle for password fields #18

Merged
merged 3 commits into from
Oct 5, 2021

Conversation

Sreejit7
Copy link
Contributor

@Sreejit7 Sreejit7 commented Oct 1, 2021

  • Added password show/hide toggles for all password and confirm password fields.

@yann-yinn I've also added a partial for password but since the field has many different fields at different I'm not sure how reusable it will be.

@yann-yinn
Copy link
Owner

yann-yinn commented Oct 1, 2021

Thanks, I will review this ASAP !

@yann-yinn
Copy link
Owner

yann-yinn commented Oct 1, 2021

hi @Sreejit7 ,
So first, amazing job, it works really well and that's a very important feature for UX, so thank you for this contribution :)

I read the PR. Here are my recommendations:

  1. major: there are JS errors because JS is included on every page, but DOM element for password does not exist on every page.

My suggestion would be to create a components for password fields, and include <script> tags directly in this new component (more on that below)

  1. minor: you've added an "id" to select fields in DOM, this is not necessary: I would suggest to target the field by its name, which is already unique and can not be changed document.querySelector("input[name='password"]'); and document.querySelector("input[name='password_confirmation"]');

I've also added a partial for password but since the field has many different fields at different I'm not sure how reusable it will be.

Password fields are the same on every page, except login page where there is no confirmation. Here is my suggestion:

  • creating a component rather than a partial , and embed the two fields + their JS in this component to fix 1) (see views/components, there is an example with FileUpload)
  • as a component is able to receive arguments, we can add an option to display / hide confirmation field (so we can use component for login form too)
    @!component('components/fieldsPassword', {
      confirmation: false,
    })

And then, in the edge component:

@if(confirmation)
// password_confirmation field
@endif

Finally I suggest using the component created to displayed password fields on every form.

Let me know what you think about all this and if you are interested to re-work some of this points, and which ones. If you don't have time, i will merge as it is, as long as 1) is fixed, and the components refacto will be done in another PR !

@Sreejit7
Copy link
Contributor Author

Sreejit7 commented Oct 3, 2021

Hi @yann-yinn , thanks for the suggestions. I appreciate your thoughts and will work on fixing 1 ASAP.
I'll also work on 2, but it will take some time because I'm new to this still. Will that be fine?

@yann-yinn
Copy link
Owner

Yep let's focus on just fixing 1) (JS errors on pages without password fields in this PR): if show / hide password is working fine on every form that's already great.

I opened another issue about refactoring password field as a component, we can do this later. #19

@yann-yinn
Copy link
Owner

Hello, I need to work on passwords this morning (validation rules / password strength), i'm merging this and will do the small fix needed !

@yann-yinn yann-yinn merged commit ec4f6eb into yann-yinn:dev Oct 5, 2021
Calcaware added a commit to Calcaware/-adonis-starter that referenced this pull request Jan 19, 2022
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