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

Partially revert listing styles changes to fix layout issues #11936

Merged
merged 1 commit into from May 17, 2024

Conversation

laymonage
Copy link
Member

@laymonage laymonage commented May 8, 2024

Redo of #11933. Fixes #11949.

In bb12877, the change of .title-wrapper from inline to inline-flex caused an issue with Safari, which b8dd7f4 tried to fix.

Then, further cleanup in 08ee15a caused an issue to the title cell. While we intended the 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). This issue technically also existed in the old --inline-actions styles before 08ee15a, but it was pretty isolated as only HistoryView used it.

bb12877 was the commit that started it all. The styling changes in there was to ensure the user avatar and the user name is vertically aligned. However, since the changes in that commit caused a chain reaction to this point, I'll try a different approach instead. Using inline-flex instead of flex for the user avatar + name wrapper seems to do the trick.

In summary

We revert the changes to listing.scss that happened in:

  • bb12877: revert the inline to inline-flex change
  • b8dd7f4: revert the align-items: center addition and .icon-folder margin adjustment removal, as well as the classes added to elements in _page_title_explore.html
  • 08ee15a: revert the flex styling added to .title (but keeping the removal of the unused --inline-actions styles

Then we slightly tweak the user_cell.html added in bb12877 to use w-inline-flex instead of w-flex for the wrapper around the user avatar and the name. The use of w-flex was what led me to make additional changes to the listing styles (changing inline to inline-flex, and all the chaos that ensued).

Comparison

Section Wagtail 6.1 This PR Wagtail 6.0
Page listing, root level image image image
Page listing, home image image image
Dashboard image image image
Workflow listing image image image
History image image image
User listing image image image

Copy link

squash-labs bot commented May 8, 2024

Manage this branch in Squash

Test this branch here: https://laymonagerevert-listing-styles-dixq0.squash.io

@laymonage laymonage changed the title Partially revert listing styles changes in b8dd7f4, 08ee15a, and bb12877 Partially revert listing styles changes to fix layout issues May 8, 2024
@laymonage laymonage self-assigned this May 8, 2024
@laymonage laymonage added type:Bug component:Universal listings Including page listings, model listings and filtering. labels May 8, 2024
@laymonage laymonage added this to the 6.1.1 milestone May 8, 2024
Comment on lines +245 to +248
.icon-folder {
margin: 3px 0.3em 0 0;
vertical-align: top;
}
Copy link
Member Author

@laymonage laymonage May 8, 2024

Choose a reason for hiding this comment

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

This was originally .icon.initial in 6.0:

.icon.initial {
margin: 3px 0.3em 0 0;
vertical-align: top;
}

but was changed to .icon-folder in d04746b (then removed in b8dd7f4).

As a result, you'll see that the globe icon for site roots is no longer aligned when compared to 6.0, since the margin is only applied for the folder icon now:

This PR

image

6.0

image

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Manual checks and visually regression test results are looking good. There is still funky alignment in a lot of places but I think this points at the need for further work, rather than changes in this PR.

Thank you @laymonage! Tested in Chrome 124, Safari 17.4, Firefox 128.



In bb12877, the change of .title-wrapper from inline to inline-flex
caused an issue with Safari, which b8dd7f4 tried to fix.

Then, further cleanup in 08ee15a caused an issue to the title cell.
While we intended the 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). This issue
  technically also existed in the old `--inline-actions` styles before
  08ee15a, but it was pretty isolated as only `HistoryView` used it.

bb12877 was the commit that started it all. The styling changes in there
was to ensure the user avatar and the user name is vertically aligned.
However, since the changes in that commit caused a chain reaction to
this point, I'll try a different approach instead. Using inline-flex
instead of flex for the user avatar + name wrapper seems to do the
trick.
@thibaudcolas thibaudcolas merged commit 8ddf472 into wagtail:main May 17, 2024
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:Universal listings Including page listings, model listings and filtering. type:Bug
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Wagtail 6.1: Chooser table cells are getting display set to flex causing cells to render in a single column
2 participants