Show "Last login" label next to the login method #622

humitos · 2025-07-02T15:35:41Z

This is just a demo of what we can do and/or how to use the last_login_method template variable. This needs more discussion and work, but opening as a draft to have something to talk over.

Email

GitHub App

Requires: readthedocs/readthedocs.org#12286
Closes #421

This is just a demo of what we can do and/or how to use the `last_login_method` template variable. Closes #421

readthedocsext/theme/templates/account/login.html

agjohnson · 2025-07-09T21:22:45Z

One concern I have about using a label and putting the label inside the menu item is that this menu already has space constraints, especially business accessing private versions:

With an added label it's going to be very tight and might wrap the menu or menu items.

I was going to use a tooltip for the menu and buttons:

<button class="ui  button" type="submit" title="GitLab" data-tooltip="Last used" data-variation="teal visible" data-position="right center">

I think even the menu labeled options could be rethought here though. We should probably just default to the correct tab, no? Do we need a label on this menu?

This is something you'd do at the template level, no JS should be required. We talked in #421 about passing some value to the templates so the templates could render the elements.

humitos · 2025-07-10T11:08:02Z

I was going to use a tooltip for the menu and buttons:

Tooltips work for me 👍🏼

I think even the menu labeled options could be rethought here though. We should probably just default to the correct tab, no? Do we need a label on this menu?

I'm already doing this in https://github.com/readthedocs/ext-theme/pull/622/files#r2194496182. Do you refer to something different here?

agjohnson · 2025-07-11T21:35:20Z

Do you refer to something different here?

Yes, specifically "do we need to put a label on the menu"? If we're defaulting is it necessary?

…last-login-method-label

Show a "Last used" tooltip on each of the buttons

humitos · 2025-07-14T12:55:06Z

OK, this is moving in a good direction 👍🏼 . I only need to attach the buttons to a specific callback to set the cookie, but all the other login to handle the variables in the template are in place:

VCS - GitHub App

Peek.2025-07-14.14-48.webm

VCS - GitLab

Email

SSO

humitos · 2025-07-14T12:56:31Z

Yes, specifically "do we need to put a label on the menu"? If we're defaulting is it necessary?

Only for email, since there is no other indicator otherwise.

humitos · 2025-07-14T15:42:42Z

I had to run the development server at localhost for the cookieStore to work. I used this diff:

diff --git a/dockerfiles/nginx/default.conf.template b/dockerfiles/nginx/default.conf.template
new file mode 100644
index 00000000..e69de29b
diff --git a/dockerfiles/nginx/web.conf.template b/dockerfiles/nginx/web.conf.template
index 9badfd39..69659d14 100644
--- a/dockerfiles/nginx/web.conf.template
+++ b/dockerfiles/nginx/web.conf.template
@@ -1,6 +1,6 @@
 server {
-    listen 80;
-    server_name $NGINX_WEB_SERVER_NAME;
+    listen 80 default_server;
+    server_name $NGINX_WEB_SERVER_NAME localhost;
 
     location /static/ {
         proxy_pass http://storage:9000/static/;
@@ -9,10 +9,10 @@ server {
 
     location / {
         proxy_pass http://web:8000/;
-        proxy_set_header Host $http_host;
+        proxy_set_header Host devthedocs.com;
+        proxy_set_header X-Forwarded-Host devthedocs.com;
         proxy_set_header X-Real-IP $remote_addr;
         proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
-        proxy_set_header X-Forwarded-Host $host;
 
         add_header X-Served Django always;
     }
@@ -30,6 +30,4 @@ server {
 
         add_header X-Served Nginx-Proxito-Sendfile always;
     }
-
-
 }
diff --git a/dockerfiles/nginx/wrangler.conf.template b/dockerfiles/nginx/wrangler.conf.template
index b99d1fae..82a6fa12 100644
--- a/dockerfiles/nginx/wrangler.conf.template
+++ b/dockerfiles/nginx/wrangler.conf.template
@@ -1,6 +1,6 @@
 # Proxito
 server {
-    listen 80 default_server;
+    listen 80;
     server_name $NGINX_PROXITO_SERVER_NAME;
 
     # Docker Compose's "logging.driver: none" is not working anymore.

humitos · 2025-07-29T08:50:33Z

@agjohnson can you take a look at this and do a final review here?

humitos · 2025-10-06T13:43:33Z

@agjohnson this is ready for review -- can you take a look?

agjohnson · 2025-10-06T20:39:21Z

readthedocsext/theme/templates/account/login.html

          <div class="right menu">
-            <button class="ui primary button" type="submit">{% trans "Log in" %}</button>
+            <button class="ui primary button"
+                    data-bind="click: $root.save_login_method"


You shouldn't use $root here, this method should be in a dedicated view class. $root is currently only used for some utility methods that are shared across many views.

agjohnson · 2025-10-06T20:40:03Z

readthedocsext/theme/templates/account/login.html

+      <a href="{% url "saml_resolve_login" %}{% if redirect_field_value %}?{{ redirect_field_name }}={{ redirect_field_value }}{% endif %}">
+
+        <button class="ui button"
+                data-bind="click: $root.save_login_method"


Another mention of $root, should be a method in a view class.

agjohnson · 2025-10-06T20:40:49Z

readthedocsext/theme/templates/socialaccount/snippets/provider_list_item.html

        action="{% provider_login_url provider.id process=process scope=scope auth_params=auth_params %}">
    {% csrf_token %}

+    {# djlint: off #}


You should use something less generic than off. If there is a rule you're trying to avoid, limit it to that rule.

agjohnson · 2025-10-06T20:41:01Z

readthedocsext/theme/templates/socialaccount/snippets/provider_list_item.html


+    {# djlint: off #}
    <button class="ui {{ button_classes }} button"
+            data-bind="click: $root.save_login_method"


Another $root

agjohnson · 2025-10-06T20:50:02Z

src/js/application/views.js

+          variation.pop("visible");
+          button.dataset.variation = variation.join(" ");
+        }
+      }


You should always use KO view code to manipulate the DOM. Using JS queries or jQuery to manipulate the DOM in addition is mixing patterns and should be always avoided.

I'm a bit confused what this is trying to do though. What is the intention here? It also seems like this would affect all modals, which we don't want.

agjohnson · 2025-10-06T20:52:12Z

src/js/application/views.js

+      cookieStore.set("last-login-method", elem.dataset.provider);
+    }
+    return true;
+  }


This should be implemented on a dedicated view class, not on the main application view. The main application view is strictly a router to these dedicated views and a couple utility methods.

You should look at any of the other templates that use a model view and work from those templates, they all follow the same pattern with a dedicated view class for each view.

agjohnson · 2025-10-06T20:53:08Z

src/js/application/views.js

+   * .. code:: html
+   *
+   *     <form method="post" action="...">
+   *       <button data-bind="click: $root.save_login_method">


Also $root here.

humitos added 2 commits July 2, 2025 17:32

Show "Last login" label next to the login method

2042860

This is just a demo of what we can do and/or how to use the `last_login_method` template variable. Closes #421

Automatically switch to Email tab when last method was email

0382b81

humitos mentioned this pull request Jul 8, 2025

Login: show "Last used" small icon when showing all the options to login #421

Open

humitos commented Jul 9, 2025

View reviewed changes

readthedocsext/theme/templates/account/login.html Outdated Show resolved Hide resolved

humitos added 5 commits July 14, 2025 13:17

Use a tooltip

05e535e

Merge branch 'main' of github.com:readthedocs/ext-theme into humitos/…

6c0da56

…last-login-method-label

Handle last_login_method and login_tab

2f839e2

Show a "Last used" tooltip on each of the buttons

Rename

af7c6e6

Tooltip position

a089234

humitos added 2 commits July 14, 2025 14:58

Bring back the beta label

5ea2c4e

Handle click on buttons to save the cookie

b57e447

humitos marked this pull request as ready for review July 14, 2025 15:21

humitos requested a review from a team as a code owner July 14, 2025 15:21

humitos requested a review from agjohnson July 14, 2025 15:21

humitos added 3 commits July 14, 2025 17:30

Small linting issues

8a5b4fa

Always show it at the right

688119a

Improve code that removes the visible property

5bece7f

agjohnson reviewed Oct 6, 2025

View reviewed changes

Show "Last login" label next to the login method #622

Are you sure you want to change the base?

Show "Last login" label next to the login method #622

Uh oh!

Conversation

humitos commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Email

GitHub App

Uh oh!

Uh oh!

agjohnson commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

humitos commented Jul 10, 2025

Uh oh!

agjohnson commented Jul 11, 2025

Uh oh!

humitos commented Jul 14, 2025

VCS - GitHub App

VCS - GitLab

Email

SSO

Uh oh!

humitos commented Jul 14, 2025

Uh oh!

humitos commented Jul 14, 2025

Uh oh!

humitos commented Jul 29, 2025

Uh oh!

humitos commented Oct 6, 2025

Uh oh!

agjohnson Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

agjohnson Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

agjohnson Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

agjohnson Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

agjohnson Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

agjohnson Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

agjohnson Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

humitos commented Jul 2, 2025 •

edited

Loading

agjohnson commented Jul 9, 2025 •

edited

Loading