Form improvements for combination repository field #678
Conversation
humitos
left a comment
humitos
left a comment
There was a problem hiding this comment.
I haven't took a look at the code yet, but screenshots look great!
Instead of using Knockout, this is using a web component to progressively enhance the fields into a single combination field. There is some underlying base classes to help make this all cleaner. Input and dropdown fields are driven by Django/Crispy, the HTML/SUI modules drive the web component instead of the web component driving the element.
|
This is settled down, I've updated the description with details. I'm not quite sure what to do with the UX around the What I did was leave the manual field enabled, but conditionally the web component won't update the repo field on remote repository field changes if this flag is enabled. I'm not really sure if that is required, but it at least allows for the fields to mismatch on submit. |
humitos
left a comment
humitos
left a comment
There was a problem hiding this comment.
This is a preliminar review, I need to take a look at the JS code still.
In the video you showed, I noticed that on validation form error the informative message saying "Additional setup steps are required" is not shown anymore. Is it possible to keep that message there when the form fails?
| {% endcomment %} | ||
|
|
||
| {# djlint wasn't happy with all of these attributes for some reason #} | ||
| {% whitespaceless as multifield_attrs %} |
There was a problem hiding this comment.
What is the as mutifield_attrs here for? I understand it gives it a name to the block, but shouldn't we use it when calling endwhitespaceless too? 🤔
|
|
||
| {# djlint wasn't happy with all of these attributes for some reason #} | ||
| {% whitespaceless as multifield_attrs %} | ||
| label="{{ multifield.label_html|safe }}" |
There was a problem hiding this comment.
Where this label HTML comes from? I'm sure @stsewd will be 😟 when seeing a |safe here 😄
There was a problem hiding this comment.
I'm also not sure from where this comes from. But yeah, we should use mark_safe or format_html from the code instead of treating the labels as safe. I looked at the django code base, and labels are never marked as safe by default (only help texts, which is weird, but probably a legacy thing).
humitos
left a comment
humitos
left a comment
There was a problem hiding this comment.
OK -- I don't have too much to say here. I don't understand 80% of the JS code from this PR, in particular src/js/modules/forms.js and I'm a little worried about maintainability.
The other file, src/js/project/admin.js is more similar to what I've been doing in addons, but there are some weird stuffs there that I don't understand either.
Summarizing, I don't feel like I can review this PR or maintain this code in the future without a pretty deep jump into it.
This repository has been increasing in complexity a lot -- I think 75% of the team still don't even understand the base semantic-ui, and we have built a few layers on top of that already, increasing its complexity.
That said, I'm 👍🏼 on what I see in the video and it looks great, but I don't feel I'm able to maintain it 😓
| */ | ||
| dispatchChangeEvent() { | ||
| this.dispatchEvent( | ||
| new CustomEvent("change", { |
There was a problem hiding this comment.
Do we want to trigger somebody else's change event or it's internal? If it's internal, I would prefix this event name with something like readthedocs-dashboard- or similar to avoid potential collisions with other things.
This work is two things: first a rework of the repo and remote_repository fields, but it also exploring using web components instead of KO. Using KO was turning into a nest of logic and hacks sprinkled through a number of templates and this is a place I'd rather we start using web components.
So there was experimenting on patterns and there are some base classes to help this. The bulk of this UI is in the multifield web component. Much of this change is explained in comments.
Here's what the UI looks like (as of 5b6add4) for first a maintainer with a connected service account and then second a maintainer without a connected service account.
Screencast.From.2026-01-15.19-05-03.mp4