Skip to content

Add a "show password" button in forms#9490

Closed
microstudi wants to merge 10 commits intodecidim:developfrom
openpoke:show-password
Closed

Add a "show password" button in forms#9490
microstudi wants to merge 10 commits intodecidim:developfrom
openpoke:show-password

Conversation

@microstudi
Copy link
Copy Markdown
Contributor

@microstudi microstudi commented Jun 27, 2022

🎩 What? Why?

This PR adds a password toggle button via unobstructive javascript (defaults to the traditional password/confirm password if javascript is disabled). This means it also hides the "confirm password" field.

To ensure full accessibility, it is based on GOV.UK article: https://technology.blog.gov.uk/2021/04/19/simple-things-are-complicated-making-a-show-password-option/
(see their implementation here: https://github.com/alphagov/govuk_publishing_components/blob/main/app/assets/javascripts/govuk_publishing_components/components/show-password.js)

Applies to:

  • User sign-up
  • Forgot password
  • Admin/forced password change
  • Account/password change
  • Set password on invitation
  • System registration
  • System edit admin

NOTE: While doing this I noticed this bug #9498 (I skipped modifying that obsolete view)

📌 Related Issues

Link your PR to an issue

Testing

Describe the best way to test or validate your PR.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing

Screenshot from 2022-06-30 12-55-51

Screenshot from 2022-06-30 12-55-45

image

image

♥️ Thank you!

@request-info
Copy link
Copy Markdown

request-info bot commented Jun 27, 2022

It seems like you didn't give us much information about what you're trying to do here. We would appreciate it if you could provide us with more info about this issue/PR!

@microstudi microstudi marked this pull request as ready for review June 28, 2022 11:57
@microstudi
Copy link
Copy Markdown
Contributor Author

This PR is ready for review @andreslucena @ahukkanen @furilo @virgile-dev
I'm mention all of you because I am not sure where this stands now that has been decided that every improvement has to match the new design. For instance, this follows the new way of creating/changing passwords but it does not use the final classes, css and html structure of the new design.

Does this means that it is still useful?
can be used as a base for the new design? (maybe @furilo can answer this one).

We have other 3 PR pending that tackles the user registrations, all of them accepted by product not long ago but, at this point we feel uncertain on how to proceed on all of them.

BTW, It's been said that any new contribution has to follow the new redesign but, how we do that if the new design is not done yet? do we have html/css guide lines already? or does this means that any new contribution has to wait until the rework done by the contractor is (almost) finished?

@virgile-dev
Copy link
Copy Markdown
Contributor

It's all good on our end with Simonas. Thanks @microstudi

@microstudi microstudi requested review from ahukkanen and andreslucena and removed request for ahukkanen June 30, 2022 19:03
@andreslucena
Copy link
Copy Markdown
Member

Hi @microstudi @virgile-dev
We've been talking with the different teams (@decidim/maintainers @decidim/product and Populate, who are working with the redesign) about the best way of tackling this, and we've come up with #9512

BTW, It's been said that any new contribution has to follow the new redesign but, how we do that if the new design is not done yet? do we have html/css guide lines already? or does this means that any new contribution has to wait until the rework done by the contractor is (almost) finished?

On this case, I'm afraid you should wait until we have merged #9455, then rebase/merge against develop and on that moment we'll be able to review and merge it all.

Mind that the plan now is to have v0.28 with the new design already. Sorry for the problems that this could mean to you guys.

@virgile-dev
Copy link
Copy Markdown
Contributor

Hello @andreslucena

I’m honestly surprised by your message.

We’ve been quite thorough with how we approached this contribution.

We’ve done extensive research (benchmark competitors, user tests, defined KPIs for success) to then publish a series of proposals with a precise scope. We’ve waited for product’s greenlight, when asked what our intentions were in term of timelines we were clear that we were aiming the next release. Again we got a green light and launched a collaboration with Ivan and Pau to get the PR ready fast to avoid creating a delay in 0.27 release.

Though I understand the redesign guidelines post 0.27 release, I’m puzzled by your message here, which goes against our previous communication with the product team. If a coordination mistake was made it’s okay but we would have appreciated you take the time to jump into a call with us to explain to us what happened.

Now we have to rescope everything into a module that will have only a 6 months utility. It implies unforeseen cost for us and our clients and blocks Ivan and Pau on a project they were planning on shipping quickly. This goes without saying these improvements which are mostly UX, not UI, will need to be recoded in the middle term future. Sounds like a waste of time for everyone.

I really hope that in the future we can have better communication between all parties and avoid creating such frustrations which make for a poor contributing experience.

@andreslucena
Copy link
Copy Markdown
Member

Sorry to hear that @virgile-dev. I have to say that these proposals were superb, and we really wanted/want to see them implemented.

I think that we need to analyze deeply what happened here. In particular, I think that we (product and the whole community) have to start thinking about what goes on in the main repository as a really slow process. We try to maintain a minimum on quality, and that doesn't play nice with deadlines.

If you have your own commitments and calendar, please don't put that pressure on the project.

Just to understand with an example that anyone can control the timings: one year ago, Barcelona had to make some developments related to the participatory budgeting. As we've seen that it was impossible to make the code, the review, and the release on time, we've forked decidim and worked on that fork just for this process (see AjuntamentdeBarcelona/decidim-barcelona#368).

This is not to put excuses, I repeat that I personally think that we need to analyze thoroughly what happened here. But it's important that everyone understand that the processes of decidim are slow, and if you have a deadline, then you need to find a solution that doesn't involve the coding/reviewing/merging/releasing process. These alternatives are well-supported by decidim: forking, making a module, or making overrides at the application level.

@ahukkanen
Copy link
Copy Markdown
Contributor

I just wanted to note here that this should wait for #9455 to be completed and merged first as otherwise it's going to be pretty hard implementing this because these pages haven't been redesigned yet.

@microstudi
Copy link
Copy Markdown
Contributor Author

As this looks it won't be merged, we've created a new module for this an other features.
https://github.com/OpenSourcePolitics/decidim-module-friendly_signup

Meanwhile we are closing this, feel free to reuse some of this code in the future if this feature is still relevant then.

andreslucena added a commit that referenced this pull request Aug 23, 2023
alecslupu added a commit that referenced this pull request Sep 10, 2023
* Remove password confirmation field

* Bring initial implementation for PasswordToggler JS class

Comes from #9490
Made by @microstudi

* Adapt PasswordToggler to redesign and call it

* Change the callers of password_field method so they all use the partial

* Add spec

* Migrate PasswordToggler to vanilla JS

* Fix specs

* Fix accessibility errors

* Refactor and clean-up javascript code

* Don't wrap the password label with the input and the button

* Fix linter offense

* Fix bug when there's an error message

* Prevent submission on enter

* Show label with required indicator (asterisk and a11y goodies)

* Fix button paddings when there's an error

* Fix failing spec when not showing help text on failed password reset

* Show required password on new session form

* Clean-up `password_required?` method

Suggested by code review

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>

---------

Co-authored-by: Alexandru Emil Lupu <contact@alecslupu.ro>
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.

Add show password option on sign in, sign up and forgot password forms

4 participants