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

Notices: Make non-dismissible notices push content down #12301

Closed
Closed
Show file tree
Hide file tree
Changes from 5 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 @@ -68,9 +68,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,
Copy link
Member

Choose a reason for hiding this comment

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

It existed before, but I don't think it's the NoticeList component itself which should need to receive a z-index, but rather its specific usage in the editor as appearing above other content. We could reflect this by applying (merging) a class name specific to the editor notice's list .editor-editor-notices__notice-list

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried removing the z-index because I agree. But then this happens:

screenshot 2018-11-27 at 10 01 51

Copy link
Member

Choose a reason for hiding this comment

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

I think it needs to exist, just not targeting NoticeList broadly, because a NoticeList could be rendered anywhere, not necessarily as we have it above content. So the z-index ought to be targeted to the more specific component which needs it (EditorNotices).

Not something which ought to be done here I think.


// Show modal under the wp-admin menus and the popover
".components-modal__screen-overlay": 100000,
Expand Down
6 changes: 6 additions & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 7.0.3 (Unreleased)

### Polish

- The `Notice` component will now always include the `.components-notice-list` class.

## 7.0.2 (2018-11-22)

## 7.0.1 (2018-11-21)
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 );
noisysocks marked this conversation as resolved.
Show resolved Hide resolved

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 @@ -80,7 +80,8 @@ function Layout( {
aria-label={ __( 'Editor content' ) }
tabIndex="-1"
>
<EditorNotices />
<EditorNotices dismissible={ false } className="is-pinned" />
<EditorNotices dismissible={ true } />
<PreserveScrollInReorder />
<EditorModeKeyboardShortcuts />
<KeyboardShortcutHelpModal />
Expand Down
44 changes: 27 additions & 17 deletions packages/edit-post/src/components/layout/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,28 @@
color: $dark-gray-900;

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

// Non-dismissible notices.
&.is-pinned.is-pinned {
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
// On mobile, toolbars behave differently.
@include break-small {
padding-top: $header-height;
}
Expand All @@ -38,7 +44,7 @@
padding-top: $block-controls-height;
}

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

Expand All @@ -53,8 +59,8 @@
}
}

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

.edit-post-layout__metaboxes:not(:empty) {
border-top: $border-width solid $light-gray-500;
Expand All @@ -79,10 +85,14 @@
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
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 );
} );
} );