FIXED #5899 -- Allowed having the collapsible fieldset and inlines ex… #19420
Conversation
There was a problem hiding this comment.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
43f109d to
63bab7a
Compare
docs/ref/contrib/admin/index.txt
Outdated
| their visibility (use the ``collapse-open`` class to have the fieldset | ||
| initially expanded). |
There was a problem hiding this comment.
The sentence seems confusing, from what I understand it's suggest using the collapse-open class instead of the collapse one. Don't we need to use both to have the detail open by default?
| their visibility (use the ``collapse-open`` class to have the fieldset | |
| initially expanded). | |
| their visibility. Add the ``collapse-open`` class to have the fieldset | |
| initially expanded. |
There was a problem hiding this comment.
We indeed need to use both to have the detail open by default. Your version is also clearer, i will modify the doc.
There was a problem hiding this comment.
I think it all boils down to whether we want users to use "classes": ["collapse", "collapse-open"] or "classes": ["collapse-open"]. Depending on that is_collapsible_open might change to check self.is_collapsible
I think it's slightly better DX to have to pick "collapse" or "collapse-open" and not both but I'm not sure how django usually handles this kind of stuff
There was a problem hiding this comment.
That's a good question, whether we need two classes or only one. I don't know if this helps, but it seems that both Bootstrap and Tailwind use two classes: one for the collapse in general and another to indicate if it's open.
There was a problem hiding this comment.
Probably a good idea to align with bootstrap and tailwind since they are heavily used. Two classes seem like a good idea 👍
docs/ref/contrib/admin/index.txt
Outdated
| class will be initially collapsed using an expandable widget (use the | ||
| ``collapse-open`` class to have the inline initially expanded). |
There was a problem hiding this comment.
The sentence seems confusing, from what I understand it's suggest using the collapse-open class instead of the collapse one. Don't we need to use both to have the detail open by default?
| class will be initially collapsed using an expandable widget (use the | |
| ``collapse-open`` class to have the inline initially expanded). | |
| class will be initially collapsed using an expandable widget. Add the | |
| ``collapse-open`` class to have the inline initially expanded. |
There was a problem hiding this comment.
We indeed need to use both to have the detail open by default. Your version is also clearer, i will modify the doc.
…nes expended on page load.
63bab7a to
b33549a
Compare
There was a problem hiding this comment.
Thank you for this @poncelettheo, I added initial comments
My main concern is this is a really old ticket and I'm not sure this is worth it/really wanted. Personally I don't see the benefit, you could just not make it collapsible in the first place (so I am 0-).
I'm not 100% sure where would be the best place to gain clarity here. We have a new feature tracker: New Features but perhaps you can discuss this on the forum https://forum.djangoproject.com/c/internals/5 to get a few more opinions (the @django/steering-council might advise)
| if any(field in self.fields for field in self.form.errors): | ||
| return False |
There was a problem hiding this comment.
| if any(field in self.fields for field in self.form.errors): | |
| return False |
We shouldn't need this
| @@ -4,7 +4,7 @@ | |||
| data-inline-type="stacked" | |||
| data-inline-formset="{{ inline_admin_formset.inline_formset_data }}"> | |||
| <fieldset class="module {{ inline_admin_formset.classes }}" aria-labelledby="{{ inline_admin_formset.formset.prefix }}-heading"> | |||
| {% if inline_admin_formset.is_collapsible %}<details><summary>{% endif %} | |||
| {% if inline_admin_formset.is_collapsible %}<details {% if inline_admin_formset.is_collapsible_open %}open{% endif %}><summary>{% endif %} | |||
There was a problem hiding this comment.
Instead of a new property we perhaps could do
| {% if inline_admin_formset.is_collapsible %}<details {% if inline_admin_formset.is_collapsible_open %}open{% endif %}><summary>{% endif %} | |
| {% if inline_admin_formset.is_collapsible %}<details {% if 'collapse-open' in inline_admin_formset.classes %}open{% endif %}><summary>{% endif %} |
| @@ -2325,7 +2326,8 @@ The ``InlineModelAdmin`` class adds or customizes: | |||
| A list or tuple containing extra CSS classes to apply to the fieldset that | |||
| is rendered for the inlines. Defaults to ``None``. As with classes | |||
| configured in :attr:`~ModelAdmin.fieldsets`, inlines with a ``collapse`` | |||
| class will be initially collapsed using an expandable widget. | |||
| class will be initially collapsed using an expandable widget. Add the | |||
| ``collapse-open`` class to have the inline initially expanded. | |||
There was a problem hiding this comment.
We need .. versionchanged:: 6.0 notes and a release note
|
This issue does is very old and I can't find any community consensus about this addition. I would agree having it go through the new-features repo would make sense. |
Allowed having the collapsible fieldsets and inlines, expended on page load.
Trac ticket number
ticket-5899
Branch description
This branch proposes to add new feature allowing to add the class
colapse-opento an admin fieldset or an inline to allow it to be expended on page load.Checklist
mainbranch.