Use better patterns for login view #671
Conversation
humitos
left a comment
humitos
left a comment
There was a problem hiding this comment.
OK... This looks good to me, but I don't fully understand it. There was no way for me to write this by myself. You are too smart 🧠
I wrote a few comments with questions to try to fully understand what's going on behind the scenes and try to figure it out everything. Hopefully, I can create the mental model and remember it next time 😄
| <i class="fas fa-shield-alt icon"></i> | ||
| {% trans "Single sign-on" %} | ||
| <div class="ui small stackable teal centered fluid wrapping menu" | ||
| data-bind="semanticui: {tabs: { autoTabActivation: last_tab() }}"> |
There was a problem hiding this comment.
last_tab() here is accessing to LoginView.last_tab variable we set before, right?
There was a problem hiding this comment.
It's not, Knockout has observable declaration order mostly handled for us, and one of the reasons we want to stick to using single patterns. The data bind layer will only be activated within KO, as the view executes, but the view will also wait for all of the view observables to resolve before allowing declaring the value to this binding.
It is possible to introduce race conditions when setting an observable value async, here you can notice the binding resolving more than once. For example an observable set from an API response, view instantiation will fire one variable change event for an undefined/default value and then another event later for the API response value.
There was a problem hiding this comment.
Just to clarify, the order of this is: everything in data-bind is evaluated after the view has been initialized, at which point the last_tab() observable has initialized with a value.
|
Excellent comments and explanation. Thanks! I still need to process all of this since I don't feel comfortable with this code. I understand it, but I don't feel I'm able to produce it by myself. I'm happy that you jumped into this task. I think we are ready to move forward here if we want and start testing this pattern in production. |
|
I'll merge down into your PR then, though sounds like that can probably be merged too, unless we need to rerun pre-commit steps there too. |
My feeback on what the LLM did to #622 is that it should use Knockout and SUI
instead of trying to continue pushing the usage into template and manual element
manipulation with JS.
This solves the UX issues with popups, SUI popups are stronger when used
directly instead of using them HTML only.
I didn't test this thoroughly.
Screencast.From.2025-11-26.20-17-02.mp4