Opened 10 years ago

Closed 8 years ago

#996 closed defect (fixed)

Different widths of input fields in edit profile

Reported by: michael Owned by:
Priority: minor Milestone:
Component: graphic design/interface Keywords: profile, bitesized
Cc: Boris Bobrov Parent Tickets:

Description

When you edit your profile you can see different widths of input fields and textareas.

If you add "width: 100%" to the input fields all input elements have the same width.

Attachments (4)

issue_996.patch (2.8 KB ) - added by Vijeth 8 years ago.
Issue 996 : Fixes fields' width consistency
issue_996_rev.patch (1.1 KB ) - added by Vijeth 8 years ago.
modify render div in util/wtforms
fixed-input-width.png (88.2 KB ) - added by Vijeth 8 years ago.
Screenshot to verify the change
0001-edit_profile.html-Consistency-in-the-width-of-input-.patch (2.1 KB ) - added by ayleph 8 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by Jeremy Pope, 10 years ago

Owner: set to Jeremy Pope
Status: newin_progress

I do have this set in the sandy theme, should apply it to the default and airy themes as well.

comment:2 by Loic Dachary, 9 years ago

Owner: Jeremy Pope removed
Status: in_progressaccepted

comment:3 by Ben Sturmfels, 8 years ago

Keywords: bitesized added

by Vijeth, 8 years ago

Attachment: issue_996.patch added

Issue 996 : Fixes fields' width consistency

comment:4 by Vijeth, 8 years ago

I'm new to MediaGlobin and this would be my first fix :)

I have attached a patch file which fixes the issue. I have currently removed the use of wtforms_util in edit_profile.html as I wanted to make width of the fields 100%. If there is a better, easier way, please let me know and I will make the required changes!

comment:5 by Vijeth, 8 years ago

Status: acceptedreview

comment:6 by ayleph, 8 years ago

Status: reviewaccepted

Hi varadhya,

Thanks for looking into this issue and writing up a patch. I don't think this is the best way forward though. We use the wtforms template to reduce code in many other templates. Your proposed change is to remove the use of wtforms in the edit_profile template. This now leaves us with two issues.

  1. Other templates that rely on wtforms still have mixed-length fields.
  1. We now have duplicated code between edit_profile and wtforms.

I think it would be best if we could continue using the wtforms template to reduce duplicated code in other templates. This helps make the code more consistent from a programming standpoint. Maybe we could modify the wtforms template itself to set the correct widths so that all of the other templates that rely on wtforms become uniform visually as well.

by Vijeth, 8 years ago

Attachment: issue_996_rev.patch added

modify render div in util/wtforms

by Vijeth, 8 years ago

Attachment: fixed-input-width.png added

Screenshot to verify the change

comment:7 by Vijeth, 8 years ago

Status: acceptedreview

Hello ayleph,

I thought of doing exactly that, but I thought it would affect other pages which use wtforms in ways it shouldn't (even in the bug description, it hasn't specified that width should be 100% for other pages also).

But, anyways sorry for the delay, I have attached a patch file which tweaks the wtforms itself (and a screenshot which verifies that) :)

Thanks!

comment:8 by ayleph, 8 years ago

Hey varadhya, thanks for making the changes. I started reviewing this but I ran into a problem. On the user registration page (mediagoblin.example.org/auth/register), there's some javascript code (show_password.js) that handles the "Show password" function. When I check the "Show password" box, the full-width password field shrinks down to a smaller size. When I uncheck the box, it grows back to 100%. I tried modifying the show_password.js file to specify 100% width for the duplicate password field, but that ends up just hiding the checkbox completely. Do you have any thoughts?

comment:9 by ayleph, 8 years ago

Well, I found a way around that, but it's not perfect. I set the duplicate password field width to 100% in show_password.js. Then, instead of creating the "Show password" checkbox in javascript, I can re-create in in the wtform view. It actually looks more consistent that way, and we can apply translation strings to it.

diff --git a/mediagoblin/plugins/basic_auth/forms.py b/mediagoblin/plugins/basic_auth/forms.py
index 3d684e91..e973290e 100644
--- a/mediagoblin/plugins/basic_auth/forms.py
+++ b/mediagoblin/plugins/basic_auth/forms.py
@@ -28,6 +28,9 @@ class RegistrationForm(wtforms.Form):
         _('Password'),
         [wtforms.validators.InputRequired(),
          wtforms.validators.Length(min=5, max=1024)])
+    password_boolean = wtforms.BooleanField(
+        label='',
+        description='Show password')
     email = wtforms.StringField(
         _('Email address'),
         [wtforms.validators.InputRequired(),
diff --git a/mediagoblin/static/js/show_password.js b/mediagoblin/static/js/show_password.js
index b3fbc862..911cce2b 100644
--- a/mediagoblin/static/js/show_password.js
+++ b/mediagoblin/static/js/show_password.js
@@ -18,7 +18,7 @@
 
 $(document).ready(function(){
   //Create a duplicate password field. We could change the input type dynamically, but this angers the IE gods (not just IE6).
-  $("#password").after('<input type="text" value="" name="password_clear" id="password_clear" /><label><input type="checkbox" id="password_boolean" />Show password</label>');
+  $("#password").after('<input type="text" value="" name="password_clear" id="password_clear" style="width:100%" />');
   $('#password_clear').hide();
   $('#password_boolean').click(function(){
     if($('#password_boolean').prop("checked")) {

But there's a downside. With the current code, the "Show password" checkbox is only shown if the user allows javascript to run. With my change above, the "Show password" checkbox is shown all the time. But if the user doesn't allow javascript to run, then the checkbox doesn't actually do anything. So that might be confusing to the user.

I guess a simple work-around would be to change the text from "Show password" to "Show password (Javascript must be enabled)" or something like that.

Version 0, edited 8 years ago by ayleph (next)

comment:10 by Vijeth, 8 years ago

Hi ayleph,


I tried modifying the show_password.js file to specify 100% width for the duplicate password field, but that ends up just hiding the checkbox completely


What do you mean hiding the checkbox though? Why can't we add a <br> statement and shift the checkbox to the next line?
IMHO it's not a good idea to mess with JS disabled scenario and put a comment like that :/

$("#password").after('<input type="text" value="" name="password_clear" id="password_clear" style="width:100%" /><label><br><input type="checkbox" id="password_boolean" />Show password</label>');

Edit : I checked it out, it works! :)

Last edited 8 years ago by Vijeth (previous) (diff)

in reply to:  10 comment:11 by ayleph, 8 years ago

Why can't we add a <br> statement and shift the checkbox to the next line?

That was my first idea too, but I couldn't make it work. I think I was putting it outside the <label> tag though, so I guess that explains it. Thanks for the suggestion! I'll continue reviewing.

comment:12 by Boris Bobrov, 8 years ago

Cc: Boris Bobrov added

comment:13 by ayleph, 8 years ago

I checked through most of the user-facing forms affected by this change and didn't see anything else that looked like it needed to be addressed. I didn't check all of the plugin templates affected by this change, as I don't have a good way to test the external authentication plugins. I've attached a slightly updated patch including the change to show_password described above. I'd go ahead and merge myself, but my computer reports that the Savannah git host machine ssh key has changed and I'd like to get confirmation that this is expected before I push.

comment:14 by ayleph, 8 years ago

Resolution: fixed
Status: reviewclosed

This has been merged in 96f66a5.

Note: See TracTickets for help on using tickets.