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
Conversation
Manage this branch in SquashTest this branch here: https://laymonagerevert-listing-styles-dixq0.squash.io |
ada3cee
to
c5c6d07
Compare
c5c6d07
to
a6e96f0
Compare
.icon-folder { | ||
margin: 3px 0.3em 0 0; | ||
vertical-align: top; | ||
} |
There was a problem hiding this comment.
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:
wagtail/client/scss/components/_listing.scss
Lines 247 to 250 in b5e068e
.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
6.0
There was a problem hiding this 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.
a6e96f0
to
7dc2cc6
Compare
Redo of #11933. Fixes #11949.
In bb12877, the change of
.title-wrapper
frominline
toinline-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:
td
andth
elementstd.title
, settingdisplay: flex
directly on thetd
element is bad, as it would cause the table layout to break in some cases (e.g. if twotd.title
elements are adjacent). This issue technically also existed in the old--inline-actions
styles before 08ee15a, but it was pretty isolated as onlyHistoryView
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 offlex
for the user avatar + name wrapper seems to do the trick.In summary
We revert the changes to
listing.scss
that happened in:inline
toinline-flex
changealign-items: center
addition and.icon-folder
margin adjustment removal, as well as the classes added to elements in_page_title_explore.html
flex
styling added to.title
(but keeping the removal of the unused--inline-actions
stylesThen we slightly tweak the
user_cell.html
added in bb12877 to usew-inline-flex
instead ofw-flex
for the wrapper around the user avatar and the name. The use ofw-flex
was what led me to make additional changes to the listing styles (changinginline
toinline-flex
, and all the chaos that ensued).Comparison