Skip to content

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

Merged
westonruter merged 1 commit intodevelopfrom
fix/admin-bar-class-removal
May 23, 2019
Merged

Remove admin-bar class name from body when admin bar element removed due to excessive CSS#2405
westonruter merged 1 commit intodevelopfrom
fix/admin-bar-class-removal

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented May 23, 2019 •
edited
Loading

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
amedina
amedina approved these changes May 23, 2019
View reviewed changes
Copy link
Copy Markdown
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
@westonruter westonruter mentioned this pull request Jun 4, 2019
Closed
5 tasks
@westonruter westonruter mentioned this pull request Sep 5, 2019
Merged
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amedina amedina amedina approved these changes

Assignees

No one assigned

Labels

cla: yes Signed the Google CLA CSS

Projects

None yet

Milestone

v1.2

Development

Successfully merging this pull request may close these issues.

3 participants

@westonruter @amedina @googlebot