Fix alignment styling issues in listings #11933
Conversation
While we intend these styles to vertically align the title cells, this wasn't a good idea for a few reasons: - The styles get applied to both td and th elements - Even if we limit this to td.title, setting display: flex directly on the td element is bad, as it would cause the table layout to break in some cases (e.g. if two td.title elements are adjacent) Remove these styles for now. This doesn't cause too much issue, since most things that need to be vertically centered are usually in a .title-wrapper element, which we have fixed in b8dd7f4. The only issue is that now the "..." actions button may not be vertically centered when the title is long. That will need to be fixed by introducing another wrapper that wraps the .title-wrapper and the .actions elements together.
Manage this branch in SquashTest this branch here: https://laymonagefix-listing-styles-t1c47.squash.io |
laymonage
force-pushed
the
fix-listing-styles
branch
3 times, most recently
from
acb0d13
to
2654cdf
Compare
| @@ -18,43 +18,45 @@ | |||
| {% for page in locked_pages %} | |||
| <tr> | |||
| <td class="title" valign="top"> | |||
| <div class="title-wrapper"> | |||
| <a href="{% url 'wagtailadmin_pages:edit' page.id %}" title="{% trans 'Edit this page' %}">{{ page.get_admin_display_title }}</a> | |||
| <div class="w-flex w-items-center w-justify-between w-gap-2"> | |||
There was a problem hiding this comment.
question: is there scope to bring these into one, more semantic class?
There was a problem hiding this comment.
I did consider it, but the current state of the HTML+CSS for the listings is very poor, and I wasn't sure if it's worth adding yet another class to listing.scss at the moment. The code badly needs a refactor to also align with our latest conventions (e.g. using w- prefix, not using float, etc.), so I chose not to add a new class for now. Wouldn't mind adding it if @thibaudcolas thinks it's a good idea though.
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| gap: theme('spacing.2'); |
There was a problem hiding this comment.
This was added in 08ee15a. I assumed that since all listing 'actions' are now inside the dropdown, we'd want to apply these vertically-centering styles to the td.title everywhere. This was wrong because:
- The styles get applied to both
td.titleandth.titleelements - Even if we limit this to
td.title, settingdisplay: flexdirectly on thetdelement is bad, as it would cause the table layout to break in some cases (e.g. if twotd.titleelements are adjacent). This issue technically also existed in the old--inline-actionsstyles before 08ee15a, but it was pretty isolated as onlyHistoryViewused it.
Remember, the main goal is to vertically center:
-
Any icons and locale tags in the title
Screenshot
-
The above, combined with the
...dropdown actions.- However, the dropdown actions button cannot be put in the same element that wraps the title and icons (
.title-wrapper), because we also want to usejustify-content: space-betweento provide the space between the title and the button (instead of usingfloat: inline-end). - As a result, the second commit in this PR adds a new wrapper to make this possible.
- However, the dropdown actions button cannot be put in the same element that wraps the title and icons (
| @@ -1,17 +1,14 @@ | |||
| {% extends "wagtailadmin/tables/title_cell.html" %} | |||
There was a problem hiding this comment.
This technically isn't a title cell but the markup used to make use of the title CSS classes for styling purposes, this went as far back as v2.16 (possibly earlier) https://static-wagtail-v2-16.netlify.app/admin/workflows/list/:
Instead of making this extend the title cell, turn this into an independent column template, and use w-label-1 on the link to achieve similar styling.
Before
After
laymonage
force-pushed
the
fix-listing-styles
branch
from
2654cdf
to
bb7cc13
Compare
laymonage
added
type:Bug
component:Universal listings
|
I'm wary that this PR changes too much and might introduce more issues than it solves. I've opted to revert the changes that caused all the issues instead, in #11936. |
See #11870 (comment) and commit messages for more details. Hide whitespace when reviewing.
Before
After