Opened 3 years ago

Closed 2 months 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: breton 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.

Subtickets

Attachments (4)

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

Download all attachments as: .zip

Change History (18)

comment:1 Changed 2 years ago by jpope

  • Owner set to jpope
  • Status changed from new to in_progress

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

comment:2 Changed 16 months ago by loic

  • Owner jpope deleted
  • Status changed from in_progress to accepted

comment:3 Changed 7 months ago by Sturm

  • Keywords bitesized added

Changed 3 months ago by varadhya

Issue 996 : Fixes fields' width consistency

comment:4 Changed 3 months ago by varadhya

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 Changed 3 months ago by varadhya

  • Status changed from accepted to review

comment:6 Changed 3 months ago by ayleph

  • Status changed from review to accepted

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.

Changed 3 months ago by varadhya

modify render div in util/wtforms

Changed 3 months ago by varadhya

Screenshot to verify the change

comment:7 Changed 3 months ago by varadhya

  • Status changed from accepted to review

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 Changed 3 months ago by ayleph

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 Changed 3 months ago by ayleph

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.

Edit: As a bonus, if we move the label text out of show_password.js, that helps with #417.

Last edited 3 months ago by ayleph (previous) (diff)

comment:10 follow-up: Changed 3 months ago by varadhya

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 3 months ago by varadhya (previous) (diff)

comment:11 in reply to: ↑ 10 Changed 3 months ago by ayleph

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 Changed 3 months ago by breton

  • Cc breton added

comment:13 Changed 2 months ago by ayleph

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 Changed 2 months ago by ayleph

  • Resolution set to fixed
  • Status changed from review to closed

This has been merged in 96f66a5.

Note: See TracTickets for help on using tickets.