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

Fixed #33407 -- Fixed .radiolist admin CSS. #15333

Merged
merged 1 commit into from Jan 26, 2022
Merged

Fixed #33407 -- Fixed .radiolist admin CSS. #15333

merged 1 commit into from Jan 26, 2022

Conversation

carltongibson
Copy link
Member

Regression in 5942ab5.

Alternate take to #15283.

This looks like the minimal change to me. Not sure if there aren't more tricky cases.

Simple model:

class NameChoices(models.TextChoices):
    JOHN = 'John'
    PAUL = "Paul"
    GEORGE = "George"
    RINGO = "Ringo"


class MyModel(models.Model):
    name = models.CharField(max_length=200, choices=NameChoices.choices)

And then an admin:

class MyModelAdmin(admin.ModelAdmin):
    radio_fields = {
        'name': admin.HORIZONTAL,
    }

Looks pretty close™ in big window, with a small margin difference below coming from the parent element in smaller viewport.

I'll upload a zip of the project to save a cycle. @matthiask — I know you exercise the admin well here, could I ask you to check?

@carltongibson
Copy link
Member Author

Test project: issue33407.zip You'll need to migrate and create a superuser.

@carltongibson carltongibson changed the title Fixed #33407 -- fixed .radiolist admin CSS. Fixed #33407 -- Fixed .radiolist admin CSS. Jan 19, 2022
@smithdc1
Copy link
Member

Hi Carlton -- Will take a look this evening.

@matthiask
Copy link
Contributor

I tested this with another project of mine and found two additional issues:

image

The line above uses two form fields in a single line; the radio field shouldn't wrap.

I've changed the line below to use admin.VERTICAL but this has no effect currently. The patch here adds support for those two:

33407-additions.txt

The whitespace looks a bit off but that's not really my area of expertise 😐 – maybe it's fine.

Comment on lines 41 to 42
overflow:visible;
display: inline-block;
Copy link
Contributor

@kezabelle kezabelle Jan 19, 2022

Choose a reason for hiding this comment

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

There may be specific reasons why introducing overflow and inline-block here make sense, but fwiw prior to moving to <div> they were float: left instead, in case that helps as a target for "how things were"

Moving to <div> > <div> also looks to have erased the following rules from being applied, which may have an effect in terms of spacing (ignoring any that might be set at responsive breakpoints!):

ul > li {
    ...
    padding: 1px 0;
}

li, dt, dd {
    font-size: 13px;
    line-height: 20px;
}

@smithdc1
Copy link
Member

Hi All,

I'm a CSS novice and an admin novice but here's my thoughts from spending time this. I've therefore spent time looking to see if it looks and behaves similar without testing if it is exactly the same.

The underlying change that caused this impacted both checkboxes and radios, yet the discussion so far has been on radios. I can see that checkboxes seem lesser used in the admin. I managed to get some with the following...

# admin.py
class HotelAdmin(admin.ModelAdmin):
    formfield_overrides = {
        models.ManyToManyField: {'widget': CheckboxSelectMultiple},
    }

admin.site.register(Hotel, HotelAdmin)

# models.py
class Hotel(models.Model):
    # I assume the Beatles used lots of hotels. 
    user = models.ManyToManyField(MyModel)
    hotel_name = models.CharField(max_length=100)
    hotel_id = models.CharField(max_length=100)

At large screens the change is subtle between 3.2 and 4.0, but something odd is happening with the proposed patch at medium screens (it looks more like 4.0 at large and smaller sizes). Sorry, I've run out of time to work out what is going on here this evening.

3.2
image

4.0
image

With proposed patch. ("medium" screen, c.950px)
image

I looked at the radios, adding back display: inline-block; as mentioned above gives me something similar to what was there before. The visual is a bit odd at different screen sizes, it jumps about a fair amount (inline with the label, under the label etc) but is what we had before, so I guess that's ok.

Copy link
Member Author

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Thanks for the reviews all!

  • The overflow wasn't needed in the end. I'd put it there to make the div take up it's layout space around the floated label child elements, but setting display achieves that. Nice. 👍
  • Good catch on the .aligned vs .inline div inline-block Matthias and Keryn.

That get's it close™.

Then there's the small margin/padding adjustments, which I added to get it as close as I could to the 3.2 output.

django/contrib/admin/static/admin/css/forms.css Outdated Show resolved Hide resolved
@kezabelle
Copy link
Contributor

kezabelle commented Jan 20, 2022

I'm not yet sure what to make of your last comment @kezabelle ref the inherited rules from ul and li elements. [...] We have a bit of time to think about that.

So, prior to 5942ab5 (i.e. at a5cb1ef) when the radiolist was <ul ... class="radiolist inline"><li>... there are 3 additional rules in the cascade, inherited from base.css (see prior comment for the selectors).

Those values (font-size, line-height and padding-top/padding-bottom) aren't overwritten before/after the ul -> div transition, so they may be of relevance to fix up any minor spacing issues. The font-size may be irrelevant because it looks to be set to the same value by the .form-row selector, and changing the line-height from it's inherited value of normal to 20px doesn't seem to change anything visually for me, but the additional padding values do make a minor difference.

Now, I'm not saying adding those values is necessarily correct, because the rest of the CSS cascade may have been adjusted since, such that introducing them is a net-negative (Blarg, CSS!). But if those account for any of the whitespace issues @matthiask was eyeballing, you can at least know (some of) where to look.


As an aside and unrelated directly to this, I think the spacing for radiolists is "wrong" at the 767px breakpoint - there's no right padding to separate values until 1024px, because it's overwritten for all .aligned label. Makes it slightly difficult to distinguish which radio has which label. On further investigation I think this replacement of the whole padding (padding: 0 0 10px;) is what's requiring you to introduce margin-top: 5px to get the right distance between the label and the radiolist itself. Something to be looked at, perhaps (perhaps independently of this though)

@smithdc1
Copy link
Member

Hi All, At circa 800px there's still some wrapping of individual inputs. Previously the group of radio buttons would shift down.

Adding inline-block to form .aligned div.radiolist seems to fix this, but I'm not sure on all of the consequences of this.

image

@carltongibson
Copy link
Member Author

carltongibson commented Jan 25, 2022

OK... 😬 :)

Thanks for the review all.

I think what we have now is Good enough™ (and indeed better than before in one case, see below) but happy to take concrete suggestions.

  • (Re-)adding the inline-block in the right places addresses most of the initial issues.

  • I looked at the inherited padding-bottom, line-height and font-size rules:

    • the latter two don't seem operative on either version (unchecking them makes no discernible difference AFAICS).
    • The padding-bottom makes a slight difference to the border-bottom position, but it's still not an exact match with 3.2, which I think would need an adjustment on div.form-row.field-name, which I'm not sure it worth it. I've currently left it out as one-less-thing.
  • I removed a margin-top: -2px that I previously had on form div.radiolist div. This brings the inputs down in relation to the label compared to 3.2 — but it aligns the baselines (exactly) which (to my eyes) looks better. I can re-add it if we think? Screenshot without those -2px, but with a guideline:

    Screenshot 2022-01-25 at 10 48 16

Putting them back would be:

diff --git a/django/contrib/admin/static/admin/css/forms.css b/django/contrib/admin/static/admin/css/forms.css
index a778057dc4..7270790797 100644
--- a/django/contrib/admin/static/admin/css/forms.css
+++ b/django/contrib/admin/static/admin/css/forms.css
@@ -38,6 +38,7 @@ label {
 /* RADIO BUTTONS */
 
 form div.radiolist div {
+    margin-top: -2px;
     padding-right: 7px;
 }

(The padding-bottom: 1px; would go in the same place.)

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@carltongibson Thanks 👍 I found tiny issues but they are not regressions.

docs/releases/4.0.2.txt Outdated Show resolved Hide resolved
@matthiask
Copy link
Contributor

Thanks! 🥳

@carltongibson carltongibson merged commit 85f2a9f into django:main Jan 26, 2022
@carltongibson carltongibson deleted the 33407-radiolist-div-css branch January 26, 2022 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants