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

Remove admin-bar class name from body when admin bar element removed due to excessive CSS #2405

Merged
merged 1 commit into from
May 23, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented May 23, 2019

Themes have styles specific for when the admin bar is shown:

twentyseventeen/style.css
1734:.admin-bar .wp-custom-header-video-button {
3505:	.admin-bar .site-navigation-fixed.navigation-top {
3743:	.admin-bar.twentyseventeen-front-page.has-header-image .custom-header-media,
3744:	.admin-bar.twentyseventeen-front-page.has-header-video .custom-header-media,
3745:	.admin-bar.home.blog.has-header-image .custom-header-media,
3746:	.admin-bar.home.blog.has-header-video .custom-header-media {
4161:	.admin-bar .site-navigation-fixed.navigation-top,
4162:	.admin-bar .site-navigation-hidden.navigation-top {

twentysixteen/style.css
2775:	body:not(.custom-background-image).admin-bar:before {
2981:	body:not(.custom-background-image).admin-bar:before {

twentynineteen/style-rtl.css
3280:.admin-bar .main-navigation .main-menu .menu-item-has-children.off-canvas .sub-menu.expanded-true {
3286:.admin-bar .main-navigation .main-menu .menu-item-has-children.off-canvas .sub-menu.expanded-true .sub-menu.expanded-true {
3291:  .admin-bar .main-navigation .main-menu .menu-item-has-children.off-canvas .sub-menu.expanded-true {
3295:  .admin-bar .main-navigation .main-menu .menu-item-has-children.off-canvas .sub-menu.expanded-true .sub-menu.expanded-true {

twentynineteen/style.css
3280:.admin-bar .main-navigation .main-menu .menu-item-has-children.off-canvas .sub-menu.expanded-true {
3286:.admin-bar .main-navigation .main-menu .menu-item-has-children.off-canvas .sub-menu.expanded-true .sub-menu.expanded-true {
3291:  .admin-bar .main-navigation .main-menu .menu-item-has-children.off-canvas .sub-menu.expanded-true {
3295:  .admin-bar .main-navigation .main-menu .menu-item-has-children.off-canvas .sub-menu.expanded-true .sub-menu.expanded-true {

twentyfourteen/style.css
3571:	.admin-bar.masthead-fixed .site-header {

With #2346 the admin bar is now removed if the admin bar CSS is excessive. However, this removal did not include the removal of the admin-bar class from the body element. The results can be undesirable:

Screen Shot 2019-05-23 at 14 11 12

The admin-bar class needs to be removed from the body in addition to the wpadminbar element being removed from the DOM. With that change applied, the result is as expected:

Screen Shot 2019-05-23 at 14 11 34

As noted in the comments, it would be nice if the style rules could be removed which have selectors referencing .admin-bar. But this would require doing a second pass at tree shaking, and it doesn't seem worthwhile since the number of styles removed is small.

@westonruter westonruter added this to the v1.2 milestone May 23, 2019
@westonruter westonruter requested a review from amedina May 23, 2019 21:17
@googlebot googlebot added the cla: yes Signed the Google CLA label May 23, 2019
Copy link
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

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

LGTM!

@westonruter westonruter merged commit 8f23abc into develop May 23, 2019
@westonruter westonruter deleted the fix/admin-bar-class-removal branch May 23, 2019 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants