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.

Sign up for GitHub

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

Components: Show popovers fullscreen on mobile #3400

Merged
merged 9 commits into from
Nov 22, 2017
Merged

Components: Show popovers fullscreen on mobile #3400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4defb1d
Components: Show popovers fullscreen on mobile
youknowriad Nov 9, 2017
1e30431
Popover: Add expandOnMobile prop
youknowriad Nov 9, 2017
a52de4f
Popovers: Fix Tooltip Width
youknowriad Nov 9, 2017
d0e6c6a
Popovere: Fix unit tests
youknowriad Nov 9, 2017
8fa3cf5
Popover: Making the `expandOnMobile` prop opt-in
youknowriad Nov 9, 2017
5960fa3
Popovers: Automatically add a close button for mobile popoveres
youknowriad Nov 13, 2017
063ea58
Popover: Avoid resetting the isMobile state when not necessary
youknowriad Nov 13, 2017
151066f
Popover: Adding a header for mobile popovers
youknowriad Nov 14, 2017
6ebaa82
Inserter: Fix Inserter styles broken after rebase
youknowriad Nov 21, 2017
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
8 changes: 8 additions & 0 deletions components/dropdown/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,11 @@ A callback invoked to render the content of the dropdown menu. Its first argumen

- Type: `Function`
- Required: Yes

## expandOnMobile

Opt-in prop to show popovers fullscreen on mobile, pass `false` in this prop to avoid this behavior.

- Type: `Boolean`
- Required: No
- Default: `false`
10 changes: 9 additions & 1 deletion components/dropdown/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,14 @@ class Dropdown extends Component {

render() {
const { isOpen } = this.state;
const { renderContent, renderToggle, position = 'bottom', className, contentClassName } = this.props;
const {
renderContent,
renderToggle,
position = 'bottom',
className,
contentClassName,
expandOnMobile,
} = this.props;
const args = { isOpen, onToggle: this.toggle, onClose: this.close };
return (
<div className={ className } ref={ this.bindContainer }>
Expand All @@ -72,6 +79,7 @@ class Dropdown extends Component {
position={ position }
onClose={ this.close }
onClickOutside={ this.clickOutside }
expandOnMobile={ expandOnMobile }
>
<FocusManaged>
{ renderContent( args ) }
Expand Down
8 changes: 8 additions & 0 deletions components/popover/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,11 @@ A callback invoked when the user clicks outside the opened popover, passing the

- Type: `Function`
- Required: No

## expandOnMobile

Opt-in prop to show popovers fullscreen on mobile, pass `false` in this prop to avoid this behavior.

- Type: `Boolean`
- Required: No
- Default: `false`
36 changes: 35 additions & 1 deletion components/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { focus, keycodes } from '@wordpress/utils';
import './style.scss';
import withFocusReturn from '../higher-order/with-focus-return';
import PopoverDetectOutside from './detect-outside';
import IconButton from '../icon-button';
import { Slot, Fill } from '../slot-fill';

const FocusManaged = withFocusReturn( ( { children } ) => children );
Expand All @@ -35,6 +36,7 @@ const ARROW_OFFSET = 20;
* @type {String}
*/
const SLOT_NAME = 'Popover';
const isMobile = () => window.innerWidth < 782;
Copy link
Member

Choose a reason for hiding this comment

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

This is one more place to sync with our scss variables. I think for now it is ok but this logic could benefit from #3331 where we add a synchronization with our scss variables, and from #3332 where we add responsive-redux making is-mobile being a prop the component receives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is, the Popover is a generic component outside the editor module. I thought about factorizing this but we can't assume any context in the components module.

Copy link
Member

@jorgefilipecosta jorgefilipecosta Nov 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Right, I would say given this it would be better to simplify the component and just receive a showFullScreen and than is up the callers to decide if is mobile and the enable this option (maybe using responsive redux) or they don't enable this option. The downside is we would need to activate this option manually in the cases we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an option but I worry about having the component's user compute the isMobile on its own. I think this would create more duplication than it's supposed to avoid especially for third-party code or WP code no in the editor module.

I personally don't mind the small duplication here to make it generic enough.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that is true it would require more effort from callers. In that case, another option would be instead of expandOnMobile we receive a prop showFullScreen that defaults to our isMobile function (If no value is passed). The advantage is that callers are able to use their own logic and their own breakpoint for mobile.


class Popover extends Component {
constructor() {
Expand All @@ -52,6 +54,7 @@ class Popover extends Component {
this.state = {
forcedYAxis: null,
forcedXAxis: null,
isMobile: false,
};
}

Expand Down Expand Up @@ -156,9 +159,28 @@ class Popover extends Component {
}

setOffset() {
const { getAnchorRect = this.getAnchorRect } = this.props;
const { getAnchorRect = this.getAnchorRect, expandOnMobile = false } = this.props;
const { popover } = this.nodes;

if ( isMobile() && expandOnMobile ) {
popover.style.left = 0;
popover.style.top = 0;
popover.style.right = 0;
popover.style.bottom = 0;
if ( ! this.state.isMobile ) {
this.setState( {
isMobile: true,
} );
}
return;
}

if ( this.state.isMobile ) {
this.setState( {
isMobile: false,
} );
}

const [ yAxis, xAxis ] = this.getPositions();
const isTop = 'top' === yAxis;
const isLeft = 'left' === xAxis;
Expand All @@ -169,6 +191,9 @@ class Popover extends Component {
return;
}

popover.style.bottom = 'auto';
popover.style.right = 'auto';

if ( isRight ) {
popover.style.left = rect.left + ARROW_OFFSET + 'px';
} else if ( isLeft ) {
Expand Down Expand Up @@ -244,6 +269,7 @@ class Popover extends Component {
range,
focusOnOpen,
getAnchorRect,
expandOnMobile,
/* eslint-enable no-unused-vars */
...contentProps
} = this.props;
Expand All @@ -258,6 +284,9 @@ class Popover extends Component {
className,
'is-' + yAxis,
'is-' + xAxis,
{
'is-mobile': this.state.isMobile,
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be state? Can't we just call isMobile()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, because we need to rerender if it changes.

}
);

// Disable reason: We care to capture the _bubbled_ events from inputs
Expand All @@ -272,6 +301,11 @@ class Popover extends Component {
{ ...contentProps }
onKeyDown={ this.maybeClose }
>
{ this.state.isMobile && (
<div className="components-popover__header">
<IconButton className="components-popover__close" icon="no-alt" onClick={ onClose } />
</div>
) }
<div
ref={ this.bindNode( 'content' ) }
className="components-popover__content"
Expand Down
126 changes: 79 additions & 47 deletions components/popover/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,90 +3,122 @@
z-index: z-index( ".components-popover" );
left: 50%;

&:before {
border: 8px solid $light-gray-500;
}

&:after {
border: 8px solid $white;
}

&:before,
&:after {
content: "";
position: absolute;
margin-left: -10px;
height: 0;
width: 0;
border-left-color: transparent;
border-right-color: transparent;
line-height: 0;
}

&.is-top {
bottom: 100%;
margin-top: -8px;

&:not(.is-mobile) {
&:before {
bottom: -8px;
border: 8px solid $light-gray-500;
}

&:after {
bottom: -6px;
border: 8px solid $white;
}

&:before,
&:after {
border-top-style: solid;
border-bottom: none;
content: "";
position: absolute;
margin-left: -10px;
height: 0;
width: 0;
border-left-color: transparent;
border-right-color: transparent;
line-height: 0;
}
}

&.is-bottom {
top: 100%;
margin-top: 8px;
&.is-top {
bottom: 100%;
margin-top: -8px;

&:before {
top: -8px;
}
&:before {
bottom: -8px;
}

&:after {
top: -6px;
&:after {
bottom: -6px;
}

&:before,
&:after {
border-top-style: solid;
border-bottom: none;
}
}

&:before,
&:after {
border-bottom-style: solid;
border-top: none;
&.is-bottom {
top: 100%;
margin-top: 8px;

&:before {
top: -8px;
}

&:after {
top: -6px;
}

&:before,
&:after {
border-bottom-style: solid;
border-top: none;
}
}
}
}

.components-popover__content {
position: absolute;
box-shadow: $shadow-popover;
border: 1px solid $light-gray-500;
background: $white;
min-width: 240px;
height: 100%;

.components-popover.is-mobile & {
height: calc( 100% - #{ $panel-header-height } );
border-top: 0;
}

.components-popover:not(.is-mobile) & {
position: absolute;
min-width: 240px;
height: auto;
}

.components-popover.is-top & {
.components-popover:not(.is-mobile).is-top & {
bottom: 100%;
}

.components-popover.is-center & {
.components-popover:not(.is-mobile).is-center & {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using a series of :not(.is-mobile) and given that is the component itself that sets the is-mobile depending on a width. I think using a CSS rule that uses our variable $break-medium may be an option to simplify this.

Copy link
Contributor Author

@youknowriad youknowriad Nov 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

is-mobile can be disabled even on mobile because this behavior is opt-out. Relying on the $break-medium would apply this consistently.

left: 50%;
transform: translateX( -50% );
}

.components-popover.is-right & {
.components-popover:not(.is-mobile).is-right & {
position: absolute;
left: 100%;
margin-left: -24px;
}

.components-popover.is-left & {
.components-popover:not(.is-mobile).is-left & {
position: absolute;
right: 100%;
margin-right: -24px;
}
}

// The withFocusReturn div
.components-popover__content > div {
height: 100%;
}

.components-popover__header {
height: $panel-header-height;
background: $light-gray-300;
padding: 0 $panel-padding;
border: 1px solid $light-gray-500;
position: relative;
}

.components-popover__close.components-icon-button {
position: absolute;
top: 6px;
right: 6px;
z-index: z-index( '.components-popover__close' );
}
4 changes: 4 additions & 0 deletions components/popover/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,8 @@ describe( 'Popover', () => {
expect( instance.nodes.popover.style ).toEqual( {
left: '225px',
top: '210px',
right: 'auto',
bottom: 'auto',
} );
} );

Expand All @@ -250,6 +252,8 @@ describe( 'Popover', () => {
expect( instance.nodes.popover.style ).toEqual( {
left: '225px',
top: '395px',
right: 'auto',
bottom: 'auto',
} );
} );
} );
Expand Down
5 changes: 3 additions & 2 deletions components/tab-panel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,12 @@ class TabPanel extends Component {
const selectedId = instanceId + '-' + selectedTab.name;

return (
<div>
<div className={ className }>
<NavigableMenu
role="tablist"
orientation={ orientation }
onNavigate={ this.onNavigate }
className={ className }
className="components-tab-panel__tabs"
>
{ tabs.map( ( tab ) => (
<TabButton className={ `${ tab.className } ${ tab.name === selected ? activeClass : '' }` }
Expand All @@ -87,6 +87,7 @@ class TabPanel extends Component {
<div aria-labelledby={ selectedId }
role="tabpanel"
id={ selectedId + '-view' }
className="components-tab-panel__tab-content"
>
{ this.props.children( selectedTab.name ) }
</div>
Expand Down
5 changes: 4 additions & 1 deletion components/tooltip/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,13 @@
}

.components-tooltip .components-popover__content {
min-width: 0;
padding: 4px 12px;
background: $dark-gray-400;
border-width: 0;
color: white;
white-space: nowrap;
}

.components-tooltip:not(.is-mobile) .components-popover__content {
min-width: 0;
}
2 changes: 1 addition & 1 deletion editor/assets/stylesheets/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ $header-height: 56px;
$inserter-tabs-height: 36px;
$block-toolbar-height: 37px;
$sidebar-width: 280px;
$sidebar-panel-header-height: 50px;
$panel-header-height: 50px;
$admin-bar-height: 32px;
$admin-bar-height-big: 46px;
$admin-sidebar-width: 160px;
Expand Down
1 change: 1 addition & 0 deletions editor/assets/stylesheets/_z-index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ $z-layers: (
'.blocks-format-toolbar__link-modal': 2,
'.editor-block-contextual-toolbar': 2,
'.editor-block-switcher__menu': 2,
'.components-popover__close': 2,
'.editor-block-mover': 1,
'.blocks-gallery-image__inline-menu': 20,
'.editor-block-settings-menu__popover': 20, // Below the header
Expand Down
1 change: 1 addition & 0 deletions editor/components/inserter/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ class Inserter extends Component {
className="editor-inserter"
position={ position }
onToggle={ this.onToggle }
expandOnMobile
renderToggle={ ( { onToggle, isOpen } ) => (
<IconButton
icon="insert"
Expand Down
Loading