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?

readthedocsext/theme/templates/account/login.html

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

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.

This was the way I found to solve this issue:

Peek.2025-11-26.17-58.webm

I'm not sure how to solve it in a better way.

Okay yeah, there are better ways to achieve that.

I think part of what is making this complicated is mixing non-Knockout JS patterns, template code, and SUI HTML-only tooltips together. The display issues seem to be from HTML-only tooltips mostly too, so maybe that isn't a good direction. Instead the solution here would be more consistent if it leaned into using Knockout more, and JS driven tooltips/popups are more robust anyways.

src/js/application/views.js

…last-login-method-label

humitos · 2025-11-26T16:08:26Z

@copilot can you help to perform the changes required from the reviewer?

Copilot · 2025-11-26T16:08:37Z

@humitos I've opened a new pull request, #670, to work on those changes. Once the pull request is ready, I'll request review from you.

Addresses reviewer feedback to move login-related JS from `ApplicationView` to a dedicated view class, following the established pattern for view separation. ### Changes - **New `LoginView` class** (`src/js/account/index.js`): Dedicated view for login functionality with `save_login_method` handler - **Remove from `ApplicationView`**: Deleted `save_login_method` and removed DOM manipulation in `show_modal` that incorrectly modified all button tooltips - **Template updates**: Wrap login content with `data-bind="using: LoginView()"` context, replace `$root.save_login_method` with `save_login_method` - **djlint**: Changed generic `{# djlint: off #}` to specific `{# djlint: off D018 #}` for template tags in attributes ### Usage ```html <div data-bind="using: LoginView()"> <button data-bind="click: save_login_method" data-provider="github"> Log in using GitHub </button> </div> ```  --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: humitos <244656+humitos@users.noreply.github.com> Co-authored-by: Manuel Kaufmann <humitos@gmail.com>

humitos · 2025-11-26T17:13:40Z

I put AI to work on it and it gave me a really good solution. There is only one comment left that we need to solve, but I'm not sure how to do what I showed in the GIF in a better way.

agjohnson

What the LLM gave back is a mixture of patterns. This can be done in a more consistent way using KO/SUI directly. See #671

agjohnson · 2025-11-26T23:30:36Z

readthedocsext/theme/templates/account/login.html

+                      data-bind="click: save_login_method"
+                      data-provider="email"


This is still working against KO a bit. We should really not need to do Event or Element manipulation to use data with KO. The two patterns that we already have available are:

data-bind="click: () => { save_login_method('email') }"

Or better if you need to do something like ensure the click event is handled normally (true is returned from the function):

data-bind="click: save_login_method.bind($data, 'email')"

Both from: https://knockoutjs.com/documentation/click-binding.html

Oh, I see. I didn't know that using a KO function in data-bind that reads data-* attributes was a mixing of patterns 👍🏼

agjohnson · 2025-11-26T23:32:43Z

readthedocsext/theme/templates/account/login.html

+                  data-bind="click: save_login_method"
+                  data-provider="sso"
+                  type="submit"
+                  {% if last_login_method == "sso" %} data-tooltip="Last used" data-variation="teal visible" data-position="right center" {% endif %}


These all need translated:

Suggested change

{% if last_login_method == "sso" %} data-tooltip="Last used" data-variation="teal visible" data-position="right center" {% endif %}

{% if last_login_method == "sso" %} data-tooltip="{% trans "Last used" %}" data-variation="teal visible" data-position="right center" {% endif %}

agjohnson · 2025-11-26T23:34:18Z

src/js/account/index.js

+ *
+ *   <div data-bind="using: LoginView()">
+ *     <form method="post" action="...">
+ *       <button data-bind="click: save_login_method" data-provider="github">


Noted above how this should probably use KO patterns instead of mixing element attribute and KO patterns. I don't see where else we're using data-provider

agjohnson · 2025-11-27T00:26:15Z

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


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


This will cause the connected service page to throw exceptions and break. That view will not have this binding, so it can only be added conditionally.

agjohnson · 2025-11-27T02:04:08Z

src/js/application/views.js

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


Okay yeah, there are better ways to achieve that.

I think part of what is making this complicated is mixing non-Knockout JS patterns, template code, and SUI HTML-only tooltips together. The display issues seem to be from HTML-only tooltips mostly too, so maybe that isn't a good direction. Instead the solution here would be more consistent if it leaned into using Knockout more, and JS driven tooltips/popups are more robust anyways.

humitos · 2025-11-27T09:42:12Z

What the LLM gave back is a mixture of patterns. This can be done in a more consistent way using KO/SUI directly

Ah, from your previous review I didn't understand we wanted to change from HTML-based tooltips to JS. By the way, the LLM did exactly what I would've done by myself.

I will take a look at your PR and see if I can continue from there.

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

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

25b5122

…last-login-method-label

Copilot AI mentioned this pull request Nov 26, 2025

Refactor login method tracking to use dedicated LoginView class #670

Merged

agjohnson mentioned this pull request Nov 27, 2025

Use better patterns for login view #671

Open

agjohnson reviewed Nov 27, 2025

View reviewed changes

	{% if last_login_method == "sso" %} data-tooltip="Last used" data-variation="teal visible" data-position="right center" {% endif %}
	{% if last_login_method == "sso" %} data-tooltip="{% trans "Last used" %}" data-variation="teal visible" data-position="right center" {% endif %}

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agjohnson Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

humitos Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

humitos commented Nov 26, 2025

Uh oh!

Copilot AI commented Nov 26, 2025

Uh oh!

humitos commented Nov 26, 2025

Uh oh!

agjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humitos Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

agjohnson Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

agjohnson Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

agjohnson Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humitos commented Nov 27, 2025

Uh oh!

Reviewers

humitos commented Jul 2, 2025 •

edited

Loading

agjohnson commented Jul 9, 2025 •

edited

Loading

agjohnson Nov 27, 2025 •

edited

Loading

agjohnson Nov 26, 2025 •

edited

Loading

agjohnson Nov 27, 2025 •

edited

Loading