Skip to content

Tabbed edit profile #184

Merged
merged 1 commit into from Mar 29, 2012

3 participants

@tallowen
Mozilla member

Rewrote to use a different bootstrap form library, used bootstrap tab
utilities.

@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
...honebook/templates/phonebook/includes/photo_form.html
@@ -0,0 +1,26 @@
+<div class="control-group photo {% if form.photo.errors %}error{% endif %}">
+ <label class="control-label">{{ form.photo.label }}</label>
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

What element is this <label> tag for?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt and 1 other commented on an outdated diff Mar 29, 2012
...honebook/templates/phonebook/includes/photo_form.html
@@ -0,0 +1,26 @@
+<div class="control-group photo {% if form.photo.errors %}error{% endif %}">
+ <label class="control-label">{{ form.photo.label }}</label>
+ <div class="controls">
+ <div class="thumbnail">
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Any reason the <img> tag itself can't have the thumbnail class? Less divitis is good.

@tallowen
Mozilla member
tallowen added a note Mar 29, 2012

Its got it to have a border that's spaced away from the actual body. Also, the #profile-photo has the whole thing with the cropping of image thumbnails and whatnot.

@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

You can do that with border: something; padding: 2px; display: [inline-]block;. But if there's other reasons OK... if it's just for a border with padding you can omit the div.

@tallowen
Mozilla member
tallowen added a note Mar 29, 2012

Since .thumbnail is defined by bootstrap I think it might just be easier to leave it the way it is - if we were using less and we could do it in a DRY way I would agree with you. Does that make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on the diff Mar 29, 2012
...honebook/templates/phonebook/includes/photo_form.html
@@ -0,0 +1,26 @@
+<div class="control-group photo {% if form.photo.errors %}error{% endif %}">
+ <label class="control-label">{{ form.photo.label }}</label>
+ <div class="controls">
+ <div class="thumbnail">
+ <img id="profile-photo" src="{{ profile.photo_url() }}" alt="{{ _('Profile photo') }}">
+ </div>
+ <ul class="photo_controls">
+ <li>
+ <input class="input-file" id="id_photo" type="file" name="photo">
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

No <label>?

@tallowen
Mozilla member
tallowen added a note Mar 29, 2012

It's has the <label> that you asked where it went. Its in a weird order to look like bram's mockup.

@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

That label tag above? It's missing its for attribute.

@tallowen
Mozilla member
tallowen added a note Mar 29, 2012

Ohh I get what you're saying :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
...honebook/templates/phonebook/includes/photo_form.html
@@ -0,0 +1,26 @@
+<div class="control-group photo {% if form.photo.errors %}error{% endif %}">
+ <label class="control-label">{{ form.photo.label }}</label>
+ <div class="controls">
+ <div class="thumbnail">
+ <img id="profile-photo" src="{{ profile.photo_url() }}" alt="{{ _('Profile photo') }}">
+ </div>
+ <ul class="photo_controls">
+ <li>
+ <input class="input-file" id="id_photo" type="file" name="photo">
+ </li>
+ {% for error in form.photo.errors %}
+ <li>
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Indent here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
...honebook/templates/phonebook/includes/photo_form.html
+ <label class="control-label">{{ form.photo.label }}</label>
+ <div class="controls">
+ <div class="thumbnail">
+ <img id="profile-photo" src="{{ profile.photo_url() }}" alt="{{ _('Profile photo') }}">
+ </div>
+ <ul class="photo_controls">
+ <li>
+ <input class="input-file" id="id_photo" type="file" name="photo">
+ </li>
+ {% for error in form.photo.errors %}
+ <li>
+ <span class="help-inline">{{ error }}</span>
+ </li>
+ {% endfor %}
+ {% if profile.photo %}
+ <li>
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Indent here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt and 1 other commented on an outdated diff Mar 29, 2012
...honebook/templates/phonebook/includes/photo_form.html
+ <img id="profile-photo" src="{{ profile.photo_url() }}" alt="{{ _('Profile photo') }}">
+ </div>
+ <ul class="photo_controls">
+ <li>
+ <input class="input-file" id="id_photo" type="file" name="photo">
+ </li>
+ {% for error in form.photo.errors %}
+ <li>
+ <span class="help-inline">{{ error }}</span>
+ </li>
+ {% endfor %}
+ {% if profile.photo %}
+ <li>
+ <label class="checkbox">
+ <input type="checkbox" name="photo-clear" id="id_photo_delete">
+ {{ _('Remove Profile Photo') }}
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Personally I don't like the container label tag, but even more so: let's be consistent and always use separate label tags like we have been doing.

@tallowen
Mozilla member
tallowen added a note Mar 29, 2012

I thought it was a thing one did for checkboxes

@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

It's not required or anything. It should work just as well outside the label tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
apps/phonebook/templates/phonebook/profile.html
@@ -33,7 +33,6 @@
{% trans profile_url=absolutify(url('profile', user.username)) %}
Your profile is waiting for approval. Send this link to someone who
is familiar with your contributions and ask them to vouch for you:
- <br>
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Does the link below appear on a newline still? I think that was the point of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
apps/users/templates/registration/register.html
{{ csrf() }}
<h1>{{ _('Create Your Profile') }}</h1>
- {{form}}
+ {{bootstrap(form)|safe}}
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Space out your moustache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
media/css/base.css
@@ -239,13 +239,26 @@ a.btn {
width:70px;
}
+.photo ul.photo_controls {
+ list-style: none;
+ float: left;
+}
+
+.photo ul.photo_controls li{
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Space before {.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
media/css/base.css
@@ -327,10 +340,18 @@ dd {
margin-left:0;
}
+.tab-content .field_description {
+ font-size: 20px;
+ margin-bottom: 20px;
+}
.search-page .result,#profile-info {
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Newline here. (Wanna add a space in after the comma while you're at it? ^_^)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
media/css/base.css
.search-page .result,#profile-info {
margin-bottom:15px;
}
+form.edit_profile {
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Do we need the form tag here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
media/css/base.css
@@ -338,3 +359,14 @@ dd {
.browserid_action_form {
display: none;
}
+
+.label-text {
+ font-family: "Helvetica Neue",Helvetica,Arial,sans-serif;
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Spaces between the fonts would be prettier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on the diff Mar 29, 2012
media/js/groups.js
@@ -1,5 +1,6 @@
(function($) {
$().ready(function() {
+ $('a[href=' + location.hash + ']').tab('show')
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

I like semicolons in my JS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt and 1 other commented on an outdated diff Mar 29, 2012
apps/phonebook/forms.py
@@ -36,7 +36,16 @@ def clean_limit(self):
return limit
-class UserForm(BootstrapModelForm):
+class UsernameWidget(forms.widgets.Input):
+ type = 'text'
+
+ def render(self, *args, **kwargs):
+ return mark_safe(u'<span class="label-text">'
+ 'http://mozillians.org/ </span>%s' %
+ super(UsernameWidget, self).render(*args, **kwargs))
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

I think this should be indented once more, right? (Will it fit?)

@tallowen
Mozilla member
tallowen added a note Mar 29, 2012

I'm not sure I follow.

@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

I thought the idea was if you can't line something up (like the strings above), indent it eight spaces beyond the base indent (which I think here would be the return). Whatever; not a biggie either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on the diff Mar 29, 2012
apps/phonebook/helpers.py
@@ -43,3 +45,16 @@ def gravatar(
'r': rating,
})
)
+
+
+@register.function
+def bootstrap(element):
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

What is all of this doing? (docstring would be nice -- but in general I don't get this)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt and 1 other commented on an outdated diff Mar 29, 2012
apps/phonebook/templates/phonebook/edit_profile.html
@@ -10,19 +10,97 @@
{% block main_content %}
<form action="{{ edit_form_action }}"
method="POST" enctype="multipart/form-data"
- class="form-horizontal">
+ class="form-horizontal edit_profile">
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Consistency in how we create class names would be nice.

@tallowen
Mozilla member
tallowen added a note Mar 29, 2012

Yeah - form-horizontal is boostrap and it seems that the rest of our classes are something_somethingelse

@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Django uses _ in its ids/names, but other than that I always used -.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
apps/phonebook/templates/phonebook/edit_profile.html
{{ csrf() }}
<h1>{{ _('Edit Your Profile') }}</h1>
-
- {{ form }}
+ <div class="tabbable">
+ <ul class="nav nav-tabs">
+ <li class="active"><a href="#1" data-toggle="tab">{{ _('Profile') }}</a></li>
+ <li><a href="#skillz" data-toggle="tab">{{ _('Skills & Groups') }}</a></li>
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

That's cute, but having it show #skills in the URL bar is probably better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on the diff Mar 29, 2012
apps/phonebook/templates/phonebook/edit_profile.html
{{ csrf() }}
<h1>{{ _('Edit Your Profile') }}</h1>
-
- {{ form }}
+ <div class="tabbable">
+ <ul class="nav nav-tabs">
+ <li class="active"><a href="#1" data-toggle="tab">{{ _('Profile') }}</a></li>
+ <li><a href="#skillz" data-toggle="tab">{{ _('Skills & Groups') }}</a></li>
+ <li><a href="#vouches" data-toggle="tab">{{ _('Vouches & Invites') }}</a></li>
+ <li><a href="#account" data-toggle="tab">{{ _('Account') }}</a></li>
+ </ul>
+ <div class="tab-content">
+ <div class="tab-pane active" id="1">
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

id="1"? Others have names.

@tallowen
Mozilla member
tallowen added a note Mar 29, 2012

its gotta for the bootstrap plugin :/

@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Man, that sucks. I really dislike that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
apps/phonebook/templates/phonebook/edit_profile.html
{{ csrf() }}
<h1>{{ _('Edit Your Profile') }}</h1>
-
- {{ form }}
+ <div class="tabbable">
+ <ul class="nav nav-tabs">
+ <li class="active"><a href="#1" data-toggle="tab">{{ _('Profile') }}</a></li>
+ <li><a href="#skillz" data-toggle="tab">{{ _('Skills & Groups') }}</a></li>
+ <li><a href="#vouches" data-toggle="tab">{{ _('Vouches & Invites') }}</a></li>
+ <li><a href="#account" data-toggle="tab">{{ _('Account') }}</a></li>
+ </ul>
+ <div class="tab-content">
+ <div class="tab-pane active" id="1">
+ {% include 'phonebook/includes/photo_form.html' %}
+ {{ bootstrap(form.first_name)|safe }}
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

What's with all the safe filters? We patch our form libraries to do that I thought? Having to |safe a bunch of stuff is pretty annoying too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
apps/phonebook/templates/phonebook/edit_profile.html
+ </div>
+ </div>
+ <div class="tab-pane" id="account">
+ {{ bootstrap(form.username)|safe }}
+ <div class="control-group">
+ <label class="control-label">{{ _('BrowserID Email') }}</label>
+ <div class="controls">
+ <span class="label-text">{{ user.email }}</span>
+ </div>
+ </div>
+ <div class="control-group">
+ <label class="control-label">{{ _('Password') }}</label>
+ <div class="controls">
+ <span class="label-text">
+ {% trans browserid='https://browserid.org/' %}
+ Change your password at <a href={{browserid}}>BrowserID</a>
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

Spaces in moustache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tofumatt tofumatt commented on an outdated diff Mar 29, 2012
apps/phonebook/templates/phonebook/edit_profile.html
+ <div class="control-group">
+ <label class="control-label">{{ _('Password') }}</label>
+ <div class="controls">
+ <span class="label-text">
+ {% trans browserid='https://browserid.org/' %}
+ Change your password at <a href={{browserid}}>BrowserID</a>
+ {% endtrans %}
+ </span>
+ </div>
+ </div>
+ <div class="control-group">
+ <label class="control-label">{{ _('Delete Account') }}</label>
+ <div class="controls">
+ <span class="label-text">
+ {% trans %}
+ If you delete your account it'll make it tougher for us to find you as a member of the Mozilla Community. You won't get to track your contributions over different sub-communities, and you'll need to regester all over with new information. Please don't go?
@tofumatt
Mozilla member
tofumatt added a note Mar 29, 2012

80 chars would be nice.

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

r- for now; let's run through the issues.

@tofumatt
Mozilla member

(Is there a bug number for this, btw?)

@tofumatt
Mozilla member

Cool: then fix bug 728877 should be in the commit message.

@tofumatt
Mozilla member

r+

Rebase and merge in.

@tallowen tallowen Tabbed edit profile
Rewrote to use a different bootstrap form library, used bootstrap tab
utilities. Fix bug 728877
ebc0b6b
@tallowen tallowen merged commit 416b812 into mozilla:master Mar 29, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.