Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed fictional class 'extrapretty' from admin docs. #18075

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adamchainz
Copy link
Sponsor Member

Trac ticket number

N/A

Branch description

The class extrapretty is a fictional example, whilst the class wide is not. Its existence in the documentation dates back to when classes were specified in a string, from a19ed8a, and it has been untouched since.

I just found a project where the example classes had been copy-pasted around the place, applying this fictional class in many admins. To avoid this kind of confusion recurring, I think we should remove it from the docs, covering only the concrete classes defined by Django.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • For UI changes, I have attached screenshots in both light and dark modes.

@@ -417,7 +417,7 @@ subclass::
Example::

{
"classes": ["wide", "extrapretty"],
"classes": ["wide"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I saw this in the docs recently I had the opposite feeling, that we should replace wide with a very obvious fictional name, such as your-custom-css-class-here:

Suggested change
"classes": ["wide"],
"classes": ["custom-class-1", "custom-class-2"],

My rationale is that the paragraph immediately following already discusses the two "useful classes defined by the default admin site stylesheet". However, since this specific example focuses on explaining how to utilize the classes attribute to apply any class defined in your CSS stylesheet, I believe it's best to keep these two ideas separate.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess wide and collapse cover 90%+ usage of this feature. It’s non-trivial to write extra CSS that fits in with all the Admin’s styles, see the provided classes:

https://github.com/django/django/blob/main/django/contrib/admin/static/admin/css/forms.css#L191-L211

So I’d prefer to present a real-world, copy-pastable example than a hypothetical one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that what wide does and what collapse does is explained after the example, in a separated paragraph. So when reading top-bottom, the user encounters:

  • explanation of the fields options dict, which shows fictional fields
  • explanation of the classes options dict, with an initial sentence reading A list or tuple containing extra CSS classes to apply to the fieldset. Example:

So at this point I think a fictional example is expected (or assumed) as well. Otherwise,if we use real admin CSS classes, the user:

  1. Will not know at the time of the (first time) reading that those are real classes
  2. May get the impression that they can only choose from those two classes

If you feel strongly about using the builtin admin CSS classes in the example, what about rewording the initial paragraph? Initial proposal:

diff --git a/docs/ref/contrib/admin/index.txt b/docs/ref/contrib/admin/index.txt
index f75b950262..c8ce4b0a57 100644
--- a/docs/ref/contrib/admin/index.txt
+++ b/docs/ref/contrib/admin/index.txt
@@ -413,18 +413,20 @@ subclass::
 
     * ``classes``
         A list or tuple containing extra CSS classes to apply to the fieldset.
+        These classes can include any custom CSS class defined in the project,
+        as well as any of the CSS classes provided by Django. Within the
+        default admin site CSS stylesheet, two particularly useful classes are
+        defined:  `collapse` and `wide`.
 
         Example::
 
             {
-                "classes": ["wide", "extrapretty"],
+                "classes": ["wide", "collapse"],
             }
 
-        Two useful classes defined by the default admin site stylesheet are
-        ``collapse`` and ``wide``. Fieldsets with the ``collapse`` style
-        will be initially collapsed in the admin and replaced with a small
-        "click to expand" link. Fieldsets with the ``wide`` style will be
-        given extra horizontal space.
+        Fieldsets with the ``collapse`` style will be initially collapsed in
+        the admin and replaced with a small "click to expand" link. Fieldsets
+        with the ``wide`` style will be given extra horizontal space.
 
     * ``description``
         A string of optional extra text to be displayed at the top of each

@nessita nessita self-requested a review April 15, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants