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
Sidebar style updates #8118
Sidebar style updates #8118
Conversation
Manage this branch in SquashTest this branch here: https://stevedyachoresidebar-style-upd-g5b2e.squash.io |
…sidebar, adjdust animations, and cleanup unused code
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.
Partial code review – will keep going tomorrow!
onClick={onClickCollapseToggle} | ||
aria-label={strings.TOGGLE_SIDEBAR} | ||
aria-expanded={slim ? 'false' : 'true'} | ||
type="button" | ||
className="button sidebar__collapse-toggle hover:w-bg-primary-200 hover:text-white hover:opacity-100" |
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.
From what I can see in the code this is white and full opaque already without hover? I can’t see a difference without those classes.
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.
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.
c-12.667,3.126-23.322,5.26-30.779,6.738C131.448,355.869,127.182,356.69,127.182,356.69"/> | ||
</g> | ||
</svg> | ||
<?xml version="1.0" encoding="utf-8"?> |
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.
@stevedya I don’t understand why there are changes to this file – if this is whitespace only, can you discard those changes?
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.
Hmm it seems to have changed the line separators from CRLF to LF. Might need your help to revert this haha i'm so confused
title={strings.EDIT_YOUR_ACCOUNT} | ||
onClick={onClickAccountSettings} | ||
aria-label={strings.EDIT_YOUR_ACCOUNT} | ||
aria-haspopup="menu" | ||
aria-expanded={accountSettingsOpen ? 'true' : 'false'} | ||
type="button" | ||
> | ||
<div className="avatar square avatar-on-dark"> | ||
<div className="avatar avatar-on-dark !w-w-[28px] !w-h-[28px]"> |
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.
I would have assumed we’d display avatars in consistent sizes elsewhere in the UI, so since there is no avatar
component in templates / React currently, we should probably do this with a --small
BEM modifier here?
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.
So there is apparently a .small
class here https://github.com/wagtail/wagtail/blob/main/client/scss/objects/_avatar.scss#L27
However it's used elsewhere in the admin, so if I was to update it, it would modify the other parts of the admin (another argument for utility styling and tailwind).
I'd vote to keep these arbitrary values here because this specific account icon is arbitrary sized itself, and only used here, so to me doesn't seem like much value to entangle the sizing here with more BEM at this point.
Let me know if I should update .small
or maybe convert it to --small
(and do a bunch of extra testing on the other spots its used)?
or
If it is okay as is?
(you know i love utils)
<div className="sidebar-footer__account-label w-label-3 w-text-white"> | ||
{user.name} | ||
</div> | ||
<Icon | ||
className="sidebar-footer__account-icon" | ||
className="w-w-4 w-h-4 w-text-white" |
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.
Considering it seems the text is white for almost everything in here, if we’re refactoring the code, I’d prefer we set w-text-white
on the whole sidebar and then use inherit
in places like this.
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.
The end result looks great @stevedya! There are a few issues with the implementation, and a few more refactorings to do, but we’re not far off.
As far as the UI, I could spot the following issues:
- On mobile viewports, the spacing around the account toggle is missing
- On mobile viewports, the hamburger button isn’t aligned with the close button
- On mobile viewports, the space for the "close" button is still there even though the button has been moved to the right side of the screen
- I can’t see the "Close" button on mobile on some pages – it’s there on
/admin/
, but not on/admin/pages/60/
. - On pages like /admin/pages/60/edit/, we’ve moved the hamburger button to the right, but the extra space needed to the left hasn’t been removed
- On a 320px viewport, the page explorer scrolls horizontally due not taking over the whole screen anymore. I think we’d want no horizontal scrolling
- In sub-menus, the menu icons don’t seem to have enough contrast against their background. We’d need a minimum of 3:1. Looks like this is because the icon has an opacity of
0.15
, on top of us changing the color to a transparent white.
And more minor things, where I might have it wrong:
- I’d expect the locale selector in the page explorer should keep having the same background as the rest of the explorer header
- I’d expect the colors of the icons in the page explorer should be more consistent
Summary GIF:
Co-authored-by: Thibaud Colas <thibaudcolas@gmail.com>
Co-authored-by: Thibaud Colas <thibaudcolas@gmail.com>
Co-authored-by: Thibaud Colas <thibaudcolas@gmail.com>
Awesome feedback! 😃 I believe I've fixed these mobile issues now.
(I should note ben does have plans to give the page explorer a proper reskin in the future however this is a temporary solution to match the new sidebar styling) |
* main: (31 commits) Removed outdated handling of length parameter to If-Modified-Since header styles - disable text-transform property-disallowed-list for required usage update @wagtail/stylelint-config-wagtail npm package disabled text-transform tailwind plugin removed redundant text-transform instances format readme with prettier Add some more dummy modules Add rule into upgrademodulepaths command for rewriting wagtail.tests imports Add some dummy modules for wagtail.tests Add a replacement rule into `wagtail updatemodulepaths` for the wagtail.core rename fix typo from wagtail#8119 prevent page refresh on enter key in header seach Add some more dummy modules for moved wagtail.core modules Update node installation instructions (wagtail#8154) Don't crash if wagtail/__init__.py is loaded without Django installed Add dummy modules to maintain wagtail.core imports Rename WagtailCoreAppConfig to WagtailAppConfig Move wagtail.core to wagtail Move tests to test Merge sites utilities into sites models ...
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.
Thank you @stevedya, all looking good now with the exception of:
I’d expect the locale selector in the page explorer should keep having the same background as the rest of the explorer header
For me it’s still a different background. I don’t want your changes to risk further conflicts anyway so will merge this now – please revisit this later.
client/tailwind.config.js
Outdated
@@ -55,6 +55,8 @@ module.exports = { | |||
/* allow system colours https://www.w3.org/TR/css-color-4/#css-system-colors */ | |||
LinkText: 'LinkText', | |||
ButtonText: 'ButtonText', | |||
white: '#FFF', | |||
black: '#000', |
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.
We already have white and black colors defined so I assume those two are useless?
Addresses #7919 styling of sidebar to match ui redesign colour scheme.
Colour changes to the page explorer portion of the sidebar are temporary to avoid major colour clashes until a new UI design for the page explorer is ready.
This doesn't address #7918 for interaction updates
Screenshots
Nested subnav
Logo on hover
With page explorer
Browsers tested on