Closed Bug 476090 Opened 15 years ago Closed 15 years ago

Allow users to log-in from toolbar

Categories

(Bugzilla :: User Interface, enhancement, P1)

3.1.2
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 3.4

People

(Reporter: guy.pyrzak, Assigned: guy.pyrzak)

References

()

Details

Attachments

(2 files, 6 obsolete files)

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.
Priority: -- → P1
Summary: Allow users to log-in and request password resets from toolbar → Allow users to log-in from toolbar
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #359702 - Flags: review?(mkanat)
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.
Attached image Screen Shot
Not sure i like how this looks but i thought I'd play around with the idea.
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?
(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.
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!
(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 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-
Attached patch V2 (obsolete) — Splinter Review
Attachment #359702 - Attachment is obsolete: true
Attachment #361914 - Flags: review?(mkanat)
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-
Attached patch V3 of patch (obsolete) — Splinter Review
Attachment #361914 - Attachment is obsolete: true
Attachment #361932 - Flags: review?(mkanat)
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-
Attached patch v4 (obsolete) — Splinter Review
Attachment #361932 - Attachment is obsolete: true
Attachment #361938 - Flags: review?(mkanat)
Attachment #361938 - Flags: review?(mkanat) → review-
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.)
Attached patch V5 (obsolete) — Splinter Review
Attachment #361938 - Attachment is obsolete: true
Attachment #361940 - Flags: review?(mkanat)
Attached patch V6 (obsolete) — Splinter Review
Attachment #361940 - Attachment is obsolete: true
Attachment #361942 - Flags: review?(mkanat)
Attachment #361940 - Flags: review?(mkanat)
Attached patch v7Splinter Review
Attachment #361942 - Attachment is obsolete: true
Attachment #361943 - Flags: review?(mkanat)
Attachment #361942 - Flags: review?(mkanat)
Attachment #361943 - Flags: review?(mkanat) → review+
Comment on attachment 361943 [details] [diff] [review]
v7

Looks great and works fine. :-)
Flags: approval+
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
Closed: 15 years ago
Resolution: --- → FIXED
Keywords: 4xp, relnote
Keywords: 4xp
Blocks: 478230
Blocks: 478232
Blocks: 478234
Blocks: 479218
Blocks: 479197
Blocks: 481910
Blocks: 482556
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.