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

adds password_confirmation field to registration page #3647

Merged
merged 3 commits into from Oct 13, 2012

Conversation

Projects
None yet
6 participants
@fabianrbz
Contributor

fabianrbz commented Oct 10, 2012

implementation for issue #3636

@jhass

This comment has been minimized.

Member

jhass commented Oct 10, 2012

Nice. Though this needs a little more fiddling than I originally thought, we need to redirect back on failure:
Try changing that line https://github.com/diaspora/diaspora/blob/develop/app/controllers/registrations_controller.rb#L24 to redirect_to :back and change this spec to define that behaviour https://github.com/diaspora/diaspora/blob/develop/spec/controllers/registrations_controller_spec.rb#L115 (.should be_redirect) or maybe it's even enough to change it to render :new, :layout => :post

Also this is a great possibility to get rid of the fixed widths there: should be as easy as removing (setting them back to auto) the width's and the margin-left from the labels and the controls divs and adding a float:right to the controls divs.

Oh and please update the Changelog :)

@jancborchardt

This comment has been minimized.

jancborchardt commented Oct 12, 2012

The original issue #3636 mentions: »Currently the user can't be sure if the entered password is correct, since the contents are hidden« – this is still the problem with 2 fields, just now with the added annoyance of needing to type blindly twice. Not only on mobile this will be a giant pain to people.

Instead, it should be done like on many common password forms and just have a »show password« checkbox which can be used to check the password before submitting: http://adactio.com/journal/1618/

@Raven24

This comment has been minimized.

Member

Raven24 commented Oct 12, 2012

while 'show password' would surely be nice to have, the password + confirmation field combination has been around for quite a while and is widely recognized as the 'classic' way to check an entered password for mistakes on sign up forms.

therefore I'd say this pull request is good (since, well, we already have it right here), and let's take it. If someone else comes along with the need for doing it the other way, I'd also be open to that.

@jancborchardt

This comment has been minimized.

jancborchardt commented Oct 12, 2012

That’s not an argument. It might have been around for a while, but it’s recognized by everyone not as the »classic«, but the annoying way to do things.

The show password pattern takes over, and beside that what we already have now (just one field) is better for users than typing it twice. How does typing blindly twice solve anything? It just creates new problems.

@jaywink

This comment has been minimized.

Contributor

jaywink commented Oct 13, 2012

IMHO don't see a need for a show password field. It's just clutter in the UI that doesn't really have any use case. IF the user manages to typo twice and cannot log in - they can always reset their password with the forgotten password feature.

Let's keep things simple?

@movilla

This comment has been minimized.

Contributor

movilla commented Oct 13, 2012

I think this change is perfect.

Perhaps the margin-top: 100px; with the inclusion of the new field is excessive. 30px I think, for example, would be nice. But this inclusion could be done later.

https://github.com/diaspora/diaspora/blob/develop/app/assets/stylesheets/new_styles/_registration.scss#L12

@jhass

This comment has been minimized.

Member

jhass commented Oct 13, 2012

More agreement than disagreement for the moment, merging. As we need to restructure our UI code anyway we can rethink the UX while doing that, but that should take place on loomio.

@jancborchardt drop me your email if you want an invite to our loomio group :)

jhass added a commit that referenced this pull request Oct 13, 2012

Merge pull request #3647 from marpo60/3636-adds-password-confirmation…
…-field

adds password_confirmation field to registration page

@jhass jhass merged commit 83c2458 into diaspora:develop Oct 13, 2012

1 check passed

default The Travis build passed
Details
@jancborchardt

This comment has been minimized.

jancborchardt commented Oct 13, 2012

Let’s step back and think about what problem we try to solve with this, regardless of implementation. We want to make sure people make less errors, right? How is that solved by introducing more potential for error?

If the problem we want to solve is a different one, please let me know. But »we already have the code« is not it. Nor is it »people are used to it« because that’s not true – services and apps move away from it because it’s a pain, and especially users with touch-devices will not want to go through typing in their password twice.

@jaywink regarding the »clutter in the UI« argument, see this mockup, which has a simple button / icon to the right side of the password field:

Windows does it too:

And Apple, albeit using the text+checkbox form:

Saying »Let’s keep things simple« I’m not sure if you agree with putting in a second field to put in the password again. How is that simple? It’s cumbersome and annoying, and also it’s »clutter in the UI«, not to speak of the interaction.

»IF the user manages to typo twice and cannot log in - they can always reset their password with the forgotten password feature.« – they can also do that when they typo only once. No need for a confirmation field.

@jancborchardt

This comment has been minimized.

jancborchardt commented Oct 13, 2012

What the? Please state what problem this addition solves.

@movilla

This comment has been minimized.

Contributor

movilla commented Oct 13, 2012

@jancborchardt, I understand your position and makes sense.

But the place to discuss things is http://loom.io/groups/194. Can we talk about this in http://loom.io/? Thanks : ]

@Raven24

This comment has been minimized.

Member

Raven24 commented Oct 13, 2012

@fabianrbz thanks for the patch! ;)

@jancborchardt please don't understand this the wrong way. historically the project was increasingly struggling to even get people involved in the first place. So now, that we finally have a steady flow of submissions we have to stay engaged and 'take what we get' - as long as it is an improvement on the status quo.
Of course, we value your input, and I see why your suggestion is ultimately what we should implement, but we simply don't have the manpower to afford discarding submissions with actual code as soon as a better idea comes along but with no code so far.

@jaywink

This comment has been minimized.

Contributor

jaywink commented Oct 13, 2012

@jancborchardt if you feel unhappy about this decision - please create a discussion about it in Loom.io and when you want you can then subject the design to a vote. However, someone must always write the code. Now the code is ready so it makes sense to go with this rather than leave it as it is. Plus there is clear concensus here on this matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment