Show "Last login" label next to the login method #622
+135
−43
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| {% trans "GitHub" as provider_name %} | ||
| <li class="item"> | ||
| <a class="ui button" | ||
| {% if last_login_method == "github" or last_login_method == "githubapp" %} data-tooltip="Last used" data-variation="teal visible" data-position="right center" {% endif %} | ||
| data-bind="click: $root.show_modal('github-select')" | ||
| title="{{ provider_name }}"> | ||
| <i class="fa-brands fa-github icon"></i> | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,15 @@ | |
| action="{% provider_login_url provider.id process=process scope=scope auth_params=auth_params %}"> | ||
| {% csrf_token %} | ||
|
|
||
| {# djlint: off D018 #} | ||
| <button class="ui {{ button_classes }} button" | ||
| data-bind="click: save_login_method" | ||
|
Contributor
There was a problem hiding this comment. 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. |
||
| data-provider="{{ provider.id }}" | ||
| type="submit" | ||
| {% if last_login_method == provider.id|lower %} data-tooltip="Last used" data-variation="teal visible" data-position="right center" {% endif %} | ||
| title="{{ provider.name }}"> | ||
| {# djlint: on #} | ||
|
|
||
| <i class="fa-brands fa-{{ provider.name|lower }} icon"></i> | ||
| {% blocktrans trimmed with provider_name=provider.app.name|default:provider.name verbiage=verbiage|default:'Connect to' %} | ||
| {{ verbiage }} {{ provider_name }} | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { Registry } from "../application/registry"; | ||
|
|
||
| /** | ||
| * LoginView manages login-related functionality including saving the | ||
| * last used login method to a cookie for session persistence. | ||
| * | ||
| * Usage in a binding context: | ||
| * | ||
| * .. code:: html | ||
| * | ||
| * <div data-bind="using: LoginView()"> | ||
| * <form method="post" action="..."> | ||
| * <button data-bind="click: save_login_method" data-provider="github"> | ||
|
Contributor
There was a problem hiding this comment. 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 |
||
| * Log in using GitHub | ||
| * </button> | ||
| * </form> | ||
| * </div> | ||
| */ | ||
| export class LoginView { | ||
| static view_name = "LoginView"; | ||
|
|
||
| constructor() {} | ||
|
|
||
| /** | ||
| * Save the provider used for login. | ||
| * | ||
| * This could be used like: | ||
| * | ||
| * .. code:: html | ||
| * | ||
| * <form method="post" action="..."> | ||
| * <button data-bind="click: save_login_method" data-provider="github"> | ||
| * Log in using GitHub | ||
| * </button> | ||
| * </form> | ||
| * | ||
| * @param {Object} data - Context data | ||
| * @param {Event} event - Click event | ||
| * @returns {knockout_click} | ||
| */ | ||
| save_login_method(data, event) { | ||
| const elem = event.currentTarget; | ||
| if (window.isSecureContext) { | ||
| console.debug("Setting last login method: ", elem.dataset.provider); | ||
| cookieStore.set("last-login-method", elem.dataset.provider); | ||
| } | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| Registry.add_view(LoginView); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import * as plugins from "./plugins"; | |
| import { Registry } from "./registry"; | ||
|
|
||
| // Application views | ||
| import * as account_views from "../account"; | ||
| import * as build_views from "../build"; | ||
| import * as core_views from "../core"; | ||
| import * as gold_views from "../gold"; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,16 @@ export class ApplicationView { | |
| if (found_modal.length === 0) { | ||
| console.debug("Modal not found:", selector); | ||
| } | ||
|
|
||
| // Remove "visible" variation property to remove "Last used" tooltip | ||
| const buttons = document.querySelectorAll(".button.ui"); | ||
| for (const button of buttons) { | ||
| if (button.dataset.variation) { | ||
| const variation = button.dataset.variation.split(" "); | ||
| variation.pop("visible"); | ||
| button.dataset.variation = variation.join(" "); | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. 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.
Member
Author
There was a problem hiding this comment. This was the way I found to solve this issue: Peek.2025-11-26.17-58.webmI'm not sure how to solve it in a better way.
Contributor
There was a problem hiding this comment. 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. |
||
| }; | ||
| } | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (
trueis returned from the function):data-bind="click: save_login_method.bind($data, 'email')"Both from: https://knockoutjs.com/documentation/click-binding.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. I didn't know that using a KO function in
data-bindthat readsdata-*attributes was a mixing of patterns 👍🏼