new TogglePassword component #1000

feymo · 2023-07-11T00:12:17Z

Q	A
Bug fix?	no
New feature?	yes
Tickets	N/A
License	MIT

Hello to the Symfony UX community !
This PR propose a new Symfony UX Component.
The TogglePassword component allows users to switch visibility on password inputs through a dedicated button.
Default style is provided and english language is used for labels but everything is customizable to fit projects requirements.

Here are screenshots previews for the default rendering :

Thanks you all in advance for your feedbacks and comments on the relevance of a component like this within Symfony UX ! 😄

Edit: The TogglePassword component propose :

A FormExtension transforming any PasswordType field into a Toggle Password by adding the toggle option ;
A form theme with complete style, English labels and SVG icons for "toggable" password field "out of the box" ;
Translation support for both toggle labels (with TranslatableMessage object or translation key string with domain) and the option to disable translation ;
Customization for : labels, icons, classes for the toggle button element, and classes for the container element.

norkunas · 2023-07-11T04:18:12Z

Icon customization could be useful too

jmsche · 2023-07-11T12:59:24Z

Hi,

I'm wondering:

could it be possible to have a form extension instead of a new form type? and so have an option available for the PasswordType like 'toggle' => true
like @norkunas, I think being allowed to customize the icons would be great
about the hidden/visible labels, it would be nice to be able to pass a TranslatableMessage that would be translated automatically, else it would require to inject the TranslatorInterface or override the template
maybe allow labels to be nullable (would display icons only)

feymo · 2023-07-11T13:19:57Z

Thanks for your feedbacks @norkunas and @jmsche ! 👍🏼
I'm working on adding icons customization as well right now

About the TranslatableMessage support, my first idea was to, when translation is used, either indeed inject TranslatorInterface in the Form object, or pass labels options from twig view using trans filter (as shown in doc/index.rst code such as

{{ form_row(form.password, { 'hidden_label': 'password_hide'|trans, 'visible_label': 'password_show'|trans }) }}

will work with provided theme. But i'll look into TranslatableMessage support, since it could ease even more translation support 👍🏼
About using a FormExtension instead of a new FormType, i chose to stick with the others packages logic (such as Dropzone) and define a new FormType. But we could totally use a FormExtension if needed or preferred, no problem for me 😃

jmsche · 2023-07-11T13:37:08Z

I think Autocomplete uses an extension as well :)

weaverryan

Honestly, this looks pretty sweet 😎. I agree with the suggestions (including the form type extension) and about making a few things more overridable + the translations stuff. Once we've got that, I'll try this out for real. But I like how this aims to just "drop in and work" for most cases but then can be configured by overriding the CSS.

Also, congrats on PR #1000! 🥳

src/TogglePassword/assets/package.json

src/TogglePassword/assets/src/controller.ts

src/TogglePassword/doc/index.rst

src/TogglePassword/assets/dist/controller.js

seb-jean · 2023-07-11T19:50:06Z

We can help with that: https://stimulus-password-visibility.stimulus-components.com/

Their template is:

<div data-controller="password-visibility">
  <input type="password" data-password-visibility-target="input" spellcheck="false" />

  <button type="button" data-action="password-visibility#toggle">
    <span data-password-visibility-target="icon">Eye</span>
    <span data-password-visibility-target="icon" class="hidden">Eye Slash</span>
  </button>
</div>

feymo · 2023-07-12T00:12:10Z

Thanks for the suggestion @seb-jean 👍🏼 but i must admit i'm not fully on board with their template approach on this one. I'd rather instantiate the stimulus controller on the password input (which is the main "actor" in this case), then create the action button and update its content without having all options loaded in the browser by default all the time. To me, the logic belongs to the stimulus controller scope, not the DOM

feymo · 2023-07-12T00:23:16Z

Based on your reviews and suggestions (thanks again for those 🙏🏼 ) :

I've changed the FormType to use a FormExtension
Icons are now customizable (and passing null hide the default icon)
Labels are now nullable too

I'm still working on the translation support for labels 👍🏼

src/TogglePassword/assets/src/controller.ts

src/TogglePassword/doc/index.rst

src/TogglePassword/src/Form/TogglePasswordTypeExtension.php

feymo · 2023-07-12T20:32:58Z

Thanks again @jmsche for your review and suggestions 👍🏼

Toggle button classlist is now customizable
Translation support has been implemented (with translation key or TranslatableMessage)
Documentation is up-to-date with the latest changes & improvements made

jmsche · 2023-07-12T20:53:44Z

Some quick notes as I'm on a phone, I'll review this more deeply tomorrow:

maybe write "button" instead of "bon" everywhere?
it's missing a .gitattributes file to lighten vendor installation
the option about classes could end with "classes" instead of "class" as it may contain several classes

Thanks for all your work :)

src/TogglePassword/.gitattributes

src/TogglePassword/src/Form/TogglePasswordTypeExtension.php

src/TogglePassword/templates/form_theme.html.twig

src/TogglePassword/assets/src/controller.ts

src/TogglePassword/doc/index.rst

jmsche

Added some minor changes.

Still missing a note in the docs about translating labels. And what about the translation domain option for labels? Would you add it, or make the developer consider using TranslatableMessage instead?

src/TogglePassword/doc/index.rst

src/TogglePassword/src/Form/TogglePasswordTypeExtension.php

feymo · 2023-07-13T10:18:00Z

Absolutely @jmsche ! Commiting your suggestion resolved the conversation and i forgot about it but i agree, i'll add a translation domain option for labels 👍🏼

src/TogglePassword/src/Form/TogglePasswordTypeExtension.php

jmsche

Seems great!

Thanks for all the changes, and sorry for all the comments 😄

feymo · 2023-07-13T12:56:58Z

No worries at all, it was super relevant and interesting ! Thank you for your time and reactivity ! 👍🏼

feymo · 2023-07-13T19:53:55Z

@weaverryan i've updated PR description to include all changes added with reviews and suggestions made, if you want to take a look and try it out, i think it looks quite clean and ready now 😄 👍🏼 🚀

weaverryan

Looking really good!

src/TogglePassword/LICENSE

src/TogglePassword/doc/index.rst

weaverryan · 2023-07-19T14:18:12Z

src/TogglePassword/assets/src/style.css

+    line-height: 1.25rem;
+    position: absolute;
+    right: 0.5rem;
+    top: -1.25rem;


This is going to be the trickiest part of this PR. Can we make some CSS that works in "most" cases. I was testing locally on https://ux.symfony.com/live-component/demos/auto-validating-form

I see the form theme template, which contains the toggle-password-container class, which is the key to making this all work "out of the box". I also see that you prepended the config to add this form theme automatically - smart! Unfortunately... because ux.symfony.com already uses a form theme, by prepending the config, the custom bootstrap_5_layout.html.twig from that project shows up last in the list, and so its form theme wins (and the one from this bundle isn't used). I also think that there should probably be a way to "opt out" of the form theme - you may want to use your normal password_widget rendering (without the new div) and then use your own CSS to place things correctly.

I'm not convinced I have the correct solution for this. But my idea is:

A) Remove the prepend and instead document the user adding the form theme manually. It kinda sucks to do this... but then the user can opt out of it.

B) Document that, instead of the form theme, you can also style manually.

Another idea, might be to:

In TogglePasswordTypeExtension, add a new toggle_form_theme option, which defaults to true.

In TogglePasswordTypeExtension, if that's true, add a new item to the block_prefixes variable - like this

ux/src/Autocomplete/src/Form/ParentEntityAutocompleteType.php

Line 56 in 24112fa

array_splice($view['autocomplete']->vars['block_prefixes'], -1, 0, 'ux_entity_autocomplete_inner');

- something like ux_password_toggle.

Change the form theme template to override just that block - e.g. ux_password_toggle_widget.

The idea would be that, if toggle_form_theme is true, then it will use your form theme. If false, it will skip and use the normal. Additionally, in your form theme (form_theme.html.twig), I believe you could then:

-{ block('form_widget') }} +{ block('password_widget') }}

So in the event the user is overriding the password_widget in a custom form theme, we will still use that.

Also, we should document how this all works a bit - basically communicating:

A) There is a custom form theme that's activated, which wraps your widget in a <div class="toggle-password-container">.
B) You can turn this off with the toggle_form_theme option, but then you're on your own to style.

Nice catch ! Indeed, that's tricky but absolutely relevant !!
I'd go for your second suggestion, adding a toggle_form_theme boolean option, which would define a block prefix 👍🏼
Let's go for that 😄

Ok, done !
I went for use_toggle_form_theme option, with true as default value so it can add a custom block prefix : ux_toggle_password (to keep consistency with the names used everywhere else in the package) and i add documentation about the behaviour to expect.
I tried to be as clear as possible on this one while while remaining as concise as possible. I hope it's clear enough, otherwise i'll try another way 😄

src/TogglePassword/assets/src/style.css

src/TogglePassword/assets/src/controller.ts

src/TogglePassword/doc/index.rst

weaverryan

Small comment - but I'm going to merge. This is very high-quality work - it was a pleasure to review and test. Thank you! Once we have a sub-tree split, would you mind creating a PR to ux.symfony.com to add a page for it? Also, once released, we should add the option to https://symfony.com/doc/current/reference/forms/types/password.html with a link over to the bundle

weaverryan · 2023-07-31T20:15:49Z

src/TogglePassword/assets/package.json

+    "types": "dist/controller.d.ts",
+    "config": {
+        "css_source": "src/style.css"
+    },


You can remove this config key. I had never noticed that we had this on a few other, old packages. I believe it was added by accident there.

sure, i'll remove it 👍🏼
I thought it was needed for the css file provided with the bundle !

It probably was in a very early WIP version of UX + Flex - that's my best guess :)

feymo · 2023-07-31T21:47:17Z

Thanks a lot to you too for your time and review @weaverryan 👍🏼
I'll sure take care of the PR for adding a page on ux.symfony.com and i'll also suggest adding a reference on the PesswordType symfony doc once released 👍🏼

weaverryan · 2023-08-01T15:27:02Z

Thank you! The sub-tree split and packagist package now exist - https://github.com/symfony/ux-toggle-password - so you should be able to create the ux.symfony.com page (we don't need a release for this - we bring in the latest, dev versions of packages inside the ux.symfony.com directory anyway).

feymo · 2023-08-01T15:29:41Z

Awesome 👍🏼 i'll create the ux.symfony.com page in a couple of hours and submit a PR for it once ready 😄

weaverryan · 2023-08-28T15:41:38Z

@feymo ux.symfony.com page is live! The release is out and the docs are up.

Would you mind creating a PR to https://github.com/symfony/symfony-docs to:

A) Document the new option (with a link to the component) on https://symfony.com/doc/current/reference/forms/types/password.html
B) Also add the component to this list - https://symfony.com/bundles/StimulusBundle/current/index.html - that's actually in this repository - we should have done it earlier :) https://github.com/symfony/stimulus-bundle/blob/2.x/doc/index.rst

Thanks!

feymo · 2023-08-28T15:50:55Z

@weaverryan Thanks ! i'll work on these as soon as possible 👍🏼

weaverryan reviewed Jul 11, 2023

View reviewed changes

jmsche suggested changes Jul 12, 2023

View reviewed changes

jmsche suggested changes Jul 13, 2023

View reviewed changes

feymo requested a review from jmsche July 13, 2023 09:57

jmsche suggested changes Jul 13, 2023

View reviewed changes

feymo requested a review from jmsche July 13, 2023 10:56

jmsche suggested changes Jul 13, 2023

View reviewed changes

src/TogglePassword/src/Form/TogglePasswordTypeExtension.php Outdated Show resolved Hide resolved

feymo requested a review from jmsche July 13, 2023 12:48

jmsche approved these changes Jul 13, 2023

View reviewed changes

weaverryan reviewed Jul 19, 2023

View reviewed changes

feymo requested a review from weaverryan July 28, 2023 18:09

weaverryan approved these changes Jul 31, 2023

View reviewed changes

new TogglePassword component

b1bf8f0

weaverryan force-pushed the feat/new-toggle-password-bundle branch from 7874579 to b1bf8f0 Compare July 31, 2023 20:26

weaverryan merged commit 1a8ea0c into symfony:2.x Jul 31, 2023

Uh oh!

new TogglePassword component #1000

new TogglePassword component #1000

Uh oh!

Conversation

feymo commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

norkunas commented Jul 11, 2023

Uh oh!

jmsche commented Jul 11, 2023

Uh oh!

feymo commented Jul 11, 2023

Uh oh!

jmsche commented Jul 11, 2023

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seb-jean commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feymo commented Jul 12, 2023

Uh oh!

feymo commented Jul 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feymo commented Jul 12, 2023

Uh oh!

jmsche commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmsche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feymo commented Jul 13, 2023

Uh oh!

Uh oh!

jmsche left a comment

Choose a reason for hiding this comment

Uh oh!

feymo commented Jul 13, 2023

Uh oh!

feymo commented Jul 13, 2023

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

weaverryan Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

feymo Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

feymo commented Jul 11, 2023 •

edited

Loading

seb-jean commented Jul 11, 2023 •

edited

Loading

jmsche commented Jul 12, 2023 •

edited

Loading