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

Make notices push down content #13614

Merged
merged 3 commits into from
Feb 14, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions assets/stylesheets/_z-index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ $z-layers: (
// but bellow #adminmenuback { z-index: 100 }
".edit-post-sidebar {greater than small}": 90,

// Show notices below expanded wp-admin submenus:
// #adminmenuwrap { z-index: 9990 }
".components-notice-list": 9989,
// Show notices below expanded editor bar
// .edit-post-header { z-index: 30 }
".components-notice-list": 29,

// Show modal under the wp-admin menus and the popover
".components-modal__screen-overlay": 100000,
Expand Down
5 changes: 4 additions & 1 deletion packages/components/src/notice/list.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/**
* External dependencies
*/
import classnames from 'classnames';
import { noop, omit } from 'lodash';

/**
Expand All @@ -18,9 +19,11 @@ import Notice from './';
* @param {Object} $0.children Array of children to be rendered inside the notice list.
* @return {Object} The rendered notices list.
*/
function NoticeList( { notices, onRemove = noop, className = 'components-notice-list', children } ) {
function NoticeList( { notices, onRemove = noop, className, children } ) {
const removeNotice = ( id ) => () => onRemove( id );

className = classnames( 'components-notice-list', className );

return (
<div className={ className }>
{ children }
Expand Down
26 changes: 26 additions & 0 deletions packages/components/src/notice/test/list.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* External dependencies
*/
import ShallowRenderer from 'react-test-renderer/shallow';

/**
* WordPress dependencies
*/
import TokenList from '@wordpress/token-list';

/**
* Internal dependencies
*/
import NoticeList from '../list';

describe( 'NoticeList', () => {
it( 'should merge className', () => {
const renderer = new ShallowRenderer();

renderer.render( <NoticeList notices={ [] } className="is-ok" /> );

const classes = new TokenList( renderer.getRenderOutput().props.className );
expect( classes.contains( 'is-ok' ) ).toBe( true );
expect( classes.contains( 'components-notice-list' ) ).toBe( true );
} );
} );
3 changes: 2 additions & 1 deletion packages/edit-post/src/components/layout/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ function Layout( {
aria-label={ __( 'Editor content' ) }
tabIndex="-1"
>
<EditorNotices />
<EditorNotices dismissible={ false } className="is-pinned" />
<EditorNotices dismissible={ true } />
<PreserveScrollInReorder />
<EditorModeKeyboardShortcuts />
<KeyboardShortcutHelpModal />
Expand Down
74 changes: 39 additions & 35 deletions packages/edit-post/src/components/layout/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -13,49 +13,36 @@
color: $dark-gray-900;

@include break-small {
position: fixed;
top: inherit;
top: 0;
}

// Non-dismissible notices.
&.is-pinned.is-pinned {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the .is-pinned to be duplicated here (specificity)? It'd be preferable if we didn't, or could find another way to override the intended "base" styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could swear it was needed, but perhaps it was needed earlier on in the development process, and as things were cleaned up it became unnecessary. In any case I tried removing the extra specificity (yes, cool new trick I learned) and it doesn't seem to have any negative effects.

position: relative;
left: 0;
top: 0;
}
}

.components-notice {
margin: 0 0 5px;
padding: 6px 12px;
min-height: $panel-header-height;
.components-notice {
margin: 0 0 5px;
padding: 6px 12px;
min-height: $panel-header-height;

.components-notice__dismiss {
margin: 10px 5px;
}
.components-notice__dismiss {
margin: 10px 5px;
}
}

// On mobile, toolbars behave differently.
// Beyond the mobile breakpoint, the editor bar is fixed, so make room for it eabove the layout content.
@include break-small {
padding-top: $header-height;
}

&.has-fixed-toolbar {
.edit-post-layout__content {
padding-top: $block-controls-height;
}

// On mobile, toolbars behave differently.
@include break-small {
padding-top: $header-height + $block-toolbar-height;

.edit-post-layout__content {
padding-top: 0;
}
}

@include break-large {
padding-top: $header-height;
}
}
// Beyond the medium breakpoint, the main scrolling area itself becomes fixed so the padding then becomes
// unnecessary, but until then it's still needed.
}

@include editor-left(".components-notice-list");
@include editor-right(".components-notice-list");

.edit-post-layout__metaboxes:not(:empty) {
border-top: $border-width solid $light-gray-500;
margin-top: 10px;
Expand Down Expand Up @@ -121,16 +108,33 @@
}
}

// For users with the Top Toolbar option enabled, special rules apply to the height of the content area.
.has-fixed-toolbar & {
// From the medium breakpoint it sits below the editor bar.
@include break-medium() {
top: $header-height + $admin-bar-height + $block-controls-height;
}

// From the xlarge breakpoint it sits in the editor bar.
@include break-xlarge() {
top: $header-height + $admin-bar-height;
}
}

// Pad the scroll box so content on the bottom can be scrolled up.
padding-bottom: 50vh;
@include break-small {
padding-bottom: 0;
}

// On mobile the main content area has to scroll otherwise you can invoke
// the overscroll bounce on the non-scrolling container, causing
// (ノಠ益ಠ)ノ彡┻━┻
overflow-y: auto;
// On mobile the main content (html or body) area has to scroll.
// If, like we do on the desktop, scroll an element (.edit-post-layout__content) you can invoke
// the overscroll bounce on the non-scrolling container, causing for a frustrating scrolling experience.
// The following rule enables this scrolling beyond the mobile breakpoint, because on the desktop
// breakpoints scrolling an isolated element helps avoid scroll bleed.
@include break-small() {
overflow-y: auto;
}
-webkit-overflow-scrolling: touch;

// This rule ensures that if you've scrolled to the end of a container,
Expand Down
1 change: 0 additions & 1 deletion packages/editor/src/components/block-list/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,6 @@

.editor-block-list__block {
.editor-block-contextual-toolbar {
position: sticky;
z-index: z-index(".editor-block-contextual-toolbar");
white-space: nowrap;
text-align: left;
Expand Down
17 changes: 14 additions & 3 deletions packages/editor/src/components/editor-notices/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import { filter } from 'lodash';

/**
* WordPress dependencies
*/
Expand All @@ -10,10 +15,16 @@ import { compose } from '@wordpress/compose';
*/
import TemplateValidationNotice from '../template-validation-notice';

function EditorNotices( props ) {
export function EditorNotices( { dismissible, notices, ...props } ) {
if ( dismissible !== undefined ) {
notices = filter( notices, { isDismissible: dismissible } );
}

return (
<NoticeList { ...props }>
<TemplateValidationNotice />
<NoticeList notices={ notices } { ...props }>
{ dismissible !== false && (
<TemplateValidationNotice />
) }
</NoticeList>
);
}
Expand Down
35 changes: 35 additions & 0 deletions packages/editor/src/components/editor-notices/test/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* External dependencies
*/
import { shallow } from 'enzyme';

/**
* Internal dependencies
*/
import { EditorNotices } from '../';

describe( 'EditorNotices', () => {
const notices = [
{ content: 'Eat your vegetables!', isDismissible: true },
{ content: 'Brush your teeth!', isDismissible: true },
{ content: 'Existence is fleeting!', isDismissible: false },
];

it( 'renders all notices', () => {
const wrapper = shallow( <EditorNotices notices={ notices } /> );
expect( wrapper.prop( 'notices' ) ).toHaveLength( 3 );
expect( wrapper.children() ).toHaveLength( 1 );
} );

it( 'renders only dismissible notices', () => {
const wrapper = shallow( <EditorNotices notices={ notices } dismissible={ true } /> );
expect( wrapper.prop( 'notices' ) ).toHaveLength( 2 );
expect( wrapper.children() ).toHaveLength( 1 );
} );

it( 'renders only non-dismissible notices', () => {
const wrapper = shallow( <EditorNotices notices={ notices } dismissible={ false } /> );
expect( wrapper.prop( 'notices' ) ).toHaveLength( 1 );
expect( wrapper.children() ).toHaveLength( 0 );
} );
} );