Allow users to log-in from toolbar

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
User Interface
P1
enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Guy Pyrzak, Assigned: Guy Pyrzak)

Tracking

3.1.2
Bugzilla 3.4
Dependency tree / graph
Bug Flags:
approval +

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

219.31 KB, image/png
Details
v7
10.88 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
Hulu has a really nice feature that lets you log in from any page on the site from the toolbar. This seems really useful and since there is no reason that i can see against us doing this and bug 475063 removes the login from the homepage I think this is both useful and a nice fix lack of login form on the homepage.

This login should bring users back to whatever page they were on before.
(Assignee)

Updated

9 years ago
Priority: -- → P1
(Assignee)

Updated

9 years ago
Summary: Allow users to log-in and request password resets from toolbar → Allow users to log-in from toolbar
(Assignee)

Comment 1

9 years ago
Created attachment 359702 [details] [diff] [review]
Patch v1
Attachment #359702 - Flags: review?(mkanat)
(Assignee)

Comment 2

9 years ago
decided to do it this way b/c i realized there wasn't a nice way to include "forgot password". Let me know if you think of anything.
(Assignee)

Comment 3

9 years ago
Created attachment 359703 [details]
Screen Shot

Not sure i like how this looks but i thought I'd play around with the idea.

Comment 4

9 years ago
Could we do it AJAX, too, since all we're doing is issuing cookies? I suppose it wouldn't change the current page, though, so we'd probably better just reload.

Can we put the form in both the header and footer?

I really like how this looks, by the way. But what's the X for?
(Assignee)

Comment 5

9 years ago
(In reply to comment #4)
> Could we do it AJAX, too, since all we're doing is issuing cookies? I suppose
> it wouldn't change the current page, though, so we'd probably better just
> reload.
Yup exactly what you said.
> 
> Can we put the form in both the header and footer?
It already is. You click "login" to make it appear however, i could just get rid of that part.
> 
> I really like how this looks, by the way. But what's the X for?
The x is for turning it back into the login text, again we can just always have it up there i just was unsure about the width of the page since the logged out page JUST so happens to be wide enough to support 800 by 600

Let me know if you want me to change the need to click the login.
(Assignee)

Comment 6

9 years ago
Oh also i realized with this change I NEED to add a forgot password link to the page as well and I'm struggling where to put it if you all have any ideas. I also fixed that bug that said the forgot login page was unclear, so make sure to check that bug out as well!

Comment 7

9 years ago
(In reply to comment #5)
> Let me know if you want me to change the need to click the login.

  Yeah, let's just have the form be visible.

(In reply to comment #6)
> Oh also i realized with this change I NEED to add a forgot password link to the
> page as well and I'm struggling where to put it if you all have any ideas.

  Perhaps you could put it into the error message that happens when somebody puts in a bad password? It's not ideal, but it at least keeps it accessible somewhere.

Comment 8

9 years ago
Comment on attachment 359702 [details] [diff] [review]
Patch v1

>+
>+.mini_login, .mini_login input, .mini_login button {
>+    font-size: xx-small;

  Wow, really? That doesn't come out as super-tiny on some browsers/displays?

>+input.bz_login {
>+    width: 15ex;

  Widths should be in em. Heights go in ex.

>+.bz_remember, .bz_restrict {

  Those seem like fairly common possible names, might want to restrict them to the form.

>+    margin: 0.2em 0;
>+    padding:  0.2em 0;

  Why both a padding and a margin?

>Index: bugzilla/template/en/default/global/common-links.html.tmpl
>       [%# If SSL is in use, use 'sslbase', else use 'urlbase'. %]
>       [% IF Param("sslbase") != "" && Param("ssl") != "never" %]
>-        [% script_name = Param("sslbase") _ script_name %]
>+        [% target = Param("sslbase") _ target %]
>       [% ELSE %]
>-        [% script_name = Param("urlbase") _ script_name %]
>-      [% END %]

  I'm pretty sure that just using [% urlbase %] (instead of the Param() business) will handle this.

>+      <li id="login_menu[% qs_suffix FILTER html %]"><span class="separator">| </span>

  I think this little login form should go into a separate file, or at least into its own block. Could we perhaps re-work login-small to use CSS and be styleable in both situations? It might not be possible--we might just need login-tiny.

>+        <a href="[% script_name FILTER html %]" 

  Don't you mean target, not script_name?

>+          [%- IF Bugzilla.cgi.param("data") %] enctype="multipart/form-data"[% END %]

  What's that for? I think we only need that if you're including hidden-fields, which we don't need to for this form, I believe...

>+            <input id="Bugzilla_login_dummy[% qs_suffix FILTER html %]" 

  Why does this exist?

>+            [% Param('emailsuffix') FILTER html %]

  You might be able to just leave that out, since we're pressed for space.

>+          <button onclick="mini_login_form('hide', '[% qs_suffix FILTER html %]');return false;">X</button>

  Yeah, we don't need this.

>Index: bugzilla/template/en/default/global/header.html.tmpl
>+<script type="text/javascript">

  Put this into a js/ file, don't put it in the templates.
Attachment #359702 - Flags: review?(mkanat) → review-
(Assignee)

Comment 9

9 years ago
Created attachment 361914 [details] [diff] [review]
V2
Attachment #359702 - Attachment is obsolete: true
Attachment #361914 - Flags: review?(mkanat)

Comment 10

9 years ago
Comment on attachment 361914 [details] [diff] [review]
V2

>? js/global.js

  Is that a file that you forgot to cvsdo add?

>Index: template/en/default/account/auth/login-small.html.tmpl

>+[% target = urlbase _ target %]
>+<li id="login_menu[% qs_suffix FILTER html %]"><span class="separator">| </span>
>+  <a href="[% target FILTER html %]" >

  That doesn't make much sense when target is index.cgi, does it? Shouldn't it have a ?GoAheadAndLogIn there or something?


>+    [%- IF Bugzilla.cgi.param("data") %] enctype="multipart/form-data"[% END %]

  I don't think we need that, since this template actually isn't used anywhere but the header and footer now.

>+      <input id="Bugzilla_login[% qs_suffix FILTER html %]" 
>+             class="bz_login bz_default_hidden"                   
>+             name="Bugzilla_login"
>+      >
>+      <input id="Bugzilla_login_dummy[% qs_suffix FILTER html %]" 

  Instead of all this dummy stuff, how about just inserting the "username" and "password" text into the fields with JS, and switching the type of the input for the password?

>+<script type="text/javascript">
>+  YAHOO.util.Event.onDOMReady(function() { 
>+    mini_login_form('show', '[% qs_suffix FILTER html %]');
>+  } );
>+</script>

  And then we can always show the form, since it doesn't really need JS to function.

>+++ js/global.js	2009-02-11 18:44:53.000000000 -0800
[snip]
>+ * The Initial Developer of the Original Code is Everything Solved, Inc.
>+ * Portions created by Everything Solved are Copyright (C) 2007 Everything
>+ * Solved, Inc. All Rights Reserved.

  Wrong Original Developer.

>+function mini_login_form( action, suffix ) {

  I think we've been mostly using camelCaps for JS functions, no?

>+        login_menu.style.display = "none";
>+        mini_login.style.display = "inline";

  Don't modify the styles directly, just add or remove the bz_default_hidden class. This allows people to modify the HTML without worrying about what block type it is or having to fix this JS (or without us worrying about whether or not the "display" attribute of a particular tag really is "inline" in all browsers).

>+function mini_login( el ) {
>+    var real = document.getElementById(el.id.replace(/_dummy/,""));
>+    el.style.display = "none";
>+    real.style.display = "inline";

  Same comment here, although I think we should get rid of the _dummy stuff, no?
Attachment #361914 - Flags: review?(mkanat) → review-
(Assignee)

Comment 11

9 years ago
Created attachment 361932 [details] [diff] [review]
V3 of patch
Attachment #361914 - Attachment is obsolete: true
Attachment #361932 - Flags: review?(mkanat)

Comment 12

9 years ago
Comment on attachment 361932 [details] [diff] [review]
V3 of patch

>+  [% target = target _ "?GoAheadAndLogIn=1" %]

  You don't need that now that target is only used as the form action. The GoAheadAndLogIn input item is enough.

>+             onfocus="mini_login_on_focus( this.id )"

  Why not just pass it "this"?

>+      [<a href="[% target FILTER html %]">options</a>]

  Maybe this should come after Log In?

>Index: template/en/default/global/common-links.html.tmpl
>+<script type="text/javascript">
>+  YAHOO.util.Event.onDOMReady(function() { 
>+    init_mini_login_form('[% qs_suffix FILTER html %]');
>+  } );
>+</script>

  Shouldn't this happen in the login-small template instead?

>+var mini_login_constants = {
>+    "login" : "login",
>+    "password" : "password"
>+}

  Oh...that needs to be in a template so that it can be localized. js files can't be localized.

>+    // check if the login and password are blank and if they are
>+    //    put in the text login and password and make them slightly greyed out

  Nit: Spacing & wording?

>+function mini_login_on_focus( el_id ) {
>+  var el = document.getElementById( el_id );
>+  if( el_id.match(/Bugzilla_password/) ){
>+      el.type = "password";
>+      if( el.value == mini_login_constants.password ) {
>+          el.value = "";

  Why not just clear it always if it's not currently password typed?

>+    window.alert("You must set the login and password before logging in.");

  That also needs to be in a template to be localizable.
Attachment #361932 - Flags: review?(mkanat) → review-
(Assignee)

Comment 13

9 years ago
Created attachment 361938 [details] [diff] [review]
v4
Attachment #361932 - Attachment is obsolete: true
Attachment #361938 - Flags: review?(mkanat)

Updated

9 years ago
Attachment #361938 - Flags: review?(mkanat) → review-

Comment 14

9 years ago
Comment on attachment 361938 [details] [diff] [review]
v4

>Index: template/en/default/account/auth/login-small.html.tmpl
>+[% IF cgi.request_method == "GET" AND cgi.query_string %]
>+  [% target = target _ "?" _ cgi.query_string %]
> [% END %]

  cgi.url can do that for you, you don't need that block.

  Everything else *looks* good. (And remove that wrong patch that I mentioned on IM.)
(Assignee)

Comment 15

9 years ago
Created attachment 361940 [details] [diff] [review]
V5
Attachment #361938 - Attachment is obsolete: true
Attachment #361940 - Flags: review?(mkanat)
(Assignee)

Comment 16

9 years ago
Created attachment 361942 [details] [diff] [review]
V6
Attachment #361940 - Attachment is obsolete: true
Attachment #361942 - Flags: review?(mkanat)
Attachment #361940 - Flags: review?(mkanat)
(Assignee)

Comment 17

9 years ago
Created attachment 361943 [details] [diff] [review]
v7
Attachment #361942 - Attachment is obsolete: true
Attachment #361943 - Flags: review?(mkanat)
Attachment #361942 - Flags: review?(mkanat)

Updated

9 years ago
Attachment #361943 - Flags: review?(mkanat) → review+

Comment 18

9 years ago
Comment on attachment 361943 [details] [diff] [review]
v7

Looks great and works fine. :-)

Updated

9 years ago
Flags: approval+

Comment 19

9 years ago
RCS file: /cvsroot/mozilla/webtools/bugzilla/js/global.js,v
done
Checking in js/global.js;
/cvsroot/mozilla/webtools/bugzilla/js/global.js,v  <--  global.js
initial revision: 1.1
done
Checking in skins/standard/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v  <--  global.css
new revision: 1.60; previous revision: 1.59
done
Checking in template/en/default/account/auth/login-small.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/auth/login-small.html.tmpl,v  <--  login-small.html.tmpl
new revision: 1.12; previous revision: 1.11
done
Checking in template/en/default/global/common-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/common-links.html.tmpl,v  <--  common-links.html.tmpl
new revision: 1.19; previous revision: 1.18
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v  <--  header.html.tmpl
new revision: 1.62; previous revision: 1.61
done
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Keywords: 4xp, relnote

Updated

9 years ago
Keywords: 4xp

Updated

9 years ago
Blocks: 478230

Updated

9 years ago
Blocks: 478232

Updated

9 years ago
Blocks: 478234
Blocks: 479218

Updated

9 years ago
Blocks: 479197

Updated

8 years ago
Blocks: 481910
(Assignee)

Updated

8 years ago
Blocks: 482556

Comment 20

8 years ago
Added to the release notes for Bugzilla 3.4 in bug 494037.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.