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

Jump to bottom

URL Input: Use Popover instead of custom positionning and custom modals #6911

Merged
merged 5 commits into from
May 24, 2018
Merged

URL Input: Use Popover instead of custom positionning and custom modals #6911

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5ab713a
URL Input: Use Popover instead of custom positionning and custom modals
youknowriad May 23, 2018
491828d
Remove the link offset
youknowriad May 23, 2018
813bd2e
Force popover rerender when we click on another link
youknowriad May 23, 2018
e483cf5
URL Input: Inheritwq focus style
youknowriad May 24, 2018
569ec28
Use the selectedNodeId instead of another popoverId
youknowriad May 24, 2018
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
2 changes: 2 additions & 0 deletions components/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ class Popover extends Component {
children,
className,
onClickOutside = onClose,
noArrow = false,
// Disable reason: We generate the `...contentProps` rest as remainder
// of props which aren't explicitly handled by this component.
/* eslint-disable no-unused-vars */
Expand Down Expand Up @@ -219,6 +220,7 @@ class Popover extends Component {
'is-' + xAxis,
{
'is-mobile': isMobile,
'no-arrow': noArrow,
Copy link
Member

@aduth aduth Jul 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is not very consistent with modifier class naming which are otherwise boolean-ish. I might have preferred something like has-no-arrow or is-without-arrow or is-no-arrow.

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 like how you see a comment from Andrew from time to time on old PRs :P

Anyway, I just pushed a fix to master (accidentally, I thought I was on a branch 😬 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3ce3fee

Copy link
Member

Choose a reason for hiding this comment

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

I enjoy to surf the git blame 😄

}
);

Expand Down
28 changes: 19 additions & 9 deletions components/popover/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
$arrow-size: 8px;

.components-popover {
position: fixed;
z-index: z-index( ".components-popover" );
Expand All @@ -10,15 +12,15 @@
bottom: 0;
}

&:not(.is-mobile) {
&:not(.no-arrow):not(.is-mobile) {
margin-left: 2px;

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

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

&:before,
Expand All @@ -34,11 +36,10 @@
}

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

&:before {
bottom: -8px;
bottom: - $arrow-size;
}

&:after {
Expand All @@ -53,12 +54,10 @@
}

&.is-bottom {
top: 100%;
margin-top: 8px;
z-index: z-index( ".components-popover.is-bottom" );

&:before {
top: -8px;
top: -$arrow-size;
}

&:after {
Expand All @@ -72,6 +71,17 @@
}
}
}

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

&.is-bottom {
top: 100%;
z-index: z-index( ".components-popover.is-bottom" );
}
}
}

.components-popover__content {
Expand Down
2 changes: 1 addition & 1 deletion core-blocks/image/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@
}
}

.editor-block-list__block[data-type="core/image"] .editor-block-toolbar .editor-format-toolbar__link-modal {
.editor-block-list__block[data-type="core/image"] .editor-block-toolbar .editor-url-input__button-modal {
position: absolute;
left: 0;
right: 0;
Expand Down
2 changes: 0 additions & 2 deletions edit-post/assets/stylesheets/_z-index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ $z-layers: (
'.editor-block-switcher__menu': 5,
'.components-popover__close': 5,
'.editor-block-list__insertion-point': 5,
'.editor-format-toolbar__link-modal': 81, // should appear above block controls
'.editor-format-toolbar__link-container': 81, // link suggestions should also
'.core-blocks-gallery-item__inline-menu': 20,
'.editor-url-input__suggestions': 30,
'.edit-post-header': 30,
Expand Down
95 changes: 50 additions & 45 deletions editor/components/rich-text/format-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
ToggleControl,
Toolbar,
withSpokenMessages,
Popover,
} from '@wordpress/components';
import { keycodes } from '@wordpress/utils';
import { prependHTTP } from '@wordpress/url';
Expand Down Expand Up @@ -62,6 +63,7 @@ class FormatToolbar extends Component {
settingsVisible: false,
opensInNewWindow: false,
linkValue: '',
popoverId: 1, // Used to force popover render to force recomputing the anchor
};

this.addLink = this.addLink.bind( this );
Expand Down Expand Up @@ -98,6 +100,7 @@ class FormatToolbar extends Component {
settingsVisible: false,
opensInNewWindow: !! nextProps.formats.link && !! nextProps.formats.link.target,
linkValue: '',
popoverId: this.state.popoverId + 1,
Copy link
Member

Choose a reason for hiding this comment

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

Could we re-use this.props.selectedNodeId instead of introducing more state? Note that RichText increments this in much the same way.

https://github.com/WordPress/gutenberg/blob/master/editor/components/rich-text/index.js#L708

} );
}
}
Expand Down Expand Up @@ -167,7 +170,7 @@ class FormatToolbar extends Component {

render() {
const { formats, focusPosition, enabledControls = DEFAULT_CONTROLS, customControls = [] } = this.props;
const { linkValue, settingsVisible, opensInNewWindow } = this.state;
const { linkValue, settingsVisible, opensInNewWindow, popoverId } = this.state;
const isAddingLink = formats.link && formats.link.isAdding;

const toolbarControls = FORMATTING_CONTROLS.concat( customControls )
Expand Down Expand Up @@ -208,58 +211,60 @@ class FormatToolbar extends Component {
{ ( isAddingLink || formats.link ) && (
<Fill name="RichText.Siblings">
<div className="editor-format-toolbar__link-container" style={ { ...focusPosition } }>
{ isAddingLink && (
<Popover position="bottom center" focusOnMount={ !! isAddingLink } key={ popoverId }>
{ isAddingLink && (
// Disable reason: KeyPress must be suppressed so the block doesn't hide the toolbar
/* eslint-disable jsx-a11y/no-noninteractive-element-interactions */
<form
className="editor-format-toolbar__link-modal"
onKeyPress={ stopKeyPropagation }
onKeyDown={ this.onKeyDown }
onSubmit={ this.submitLink }>
<div className="editor-format-toolbar__link-modal-line">
<UrlInput value={ linkValue } onChange={ this.onChangeLinkValue } />
<IconButton icon="editor-break" label={ __( 'Apply' ) } type="submit" />
<IconButton
className="editor-format-toolbar__link-settings-toggle"
icon="ellipsis"
label={ __( 'Link Settings' ) }
onClick={ this.toggleLinkSettingsVisibility }
aria-expanded={ settingsVisible }
/>
</div>
{ linkSettings }
</form>
<form
className="editor-format-toolbar__link-modal"
onKeyPress={ stopKeyPropagation }
onKeyDown={ this.onKeyDown }
onSubmit={ this.submitLink }>
<div className="editor-format-toolbar__link-modal-line">
<UrlInput value={ linkValue } onChange={ this.onChangeLinkValue } />
<IconButton icon="editor-break" label={ __( 'Apply' ) } type="submit" />
<IconButton
className="editor-format-toolbar__link-settings-toggle"
icon="ellipsis"
label={ __( 'Link Settings' ) }
onClick={ this.toggleLinkSettingsVisibility }
aria-expanded={ settingsVisible }
/>
</div>
{ linkSettings }
</form>
/* eslint-enable jsx-a11y/no-noninteractive-element-interactions */
) }
) }

{ formats.link && ! isAddingLink && (
{ formats.link && ! isAddingLink && (
// Disable reason: KeyPress must be suppressed so the block doesn't hide the toolbar
/* eslint-disable jsx-a11y/no-static-element-interactions */
<div
className="editor-format-toolbar__link-modal"
onKeyPress={ stopKeyPropagation }
>
<div className="editor-format-toolbar__link-modal-line">
<a
className="editor-format-toolbar__link-value"
href={ formats.link.value }
target="_blank"
>
{ formats.link.value && filterURLForDisplay( decodeURI( formats.link.value ) ) }
</a>
<IconButton icon="edit" label={ __( 'Edit' ) } onClick={ this.editLink } />
<IconButton
className="editor-format-toolbar__link-settings-toggle"
icon="ellipsis"
label={ __( 'Link Settings' ) }
onClick={ this.toggleLinkSettingsVisibility }
aria-expanded={ settingsVisible }
/>
<div
className="editor-format-toolbar__link-modal"
onKeyPress={ stopKeyPropagation }
>
<div className="editor-format-toolbar__link-modal-line">
<a
className="editor-format-toolbar__link-value"
href={ formats.link.value }
target="_blank"
>
{ formats.link.value && filterURLForDisplay( decodeURI( formats.link.value ) ) }
</a>
<IconButton icon="edit" label={ __( 'Edit' ) } onClick={ this.editLink } />
<IconButton
className="editor-format-toolbar__link-settings-toggle"
icon="ellipsis"
label={ __( 'Link Settings' ) }
onClick={ this.toggleLinkSettingsVisibility }
aria-expanded={ settingsVisible }
/>
</div>
{ linkSettings }
</div>
{ linkSettings }
</div>
/* eslint-enable jsx-a11y/no-static-element-interactions */
) }
) }
</Popover>
</div>
</Fill>
) }
Expand Down
26 changes: 4 additions & 22 deletions editor/components/rich-text/format-toolbar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,14 @@
display: inline-flex;
}

.editor-format-toolbar__link-modal {
position: relative;
box-shadow: $shadow-popover;
border: 1px solid $light-gray-500;
background: $white;
display: flex;
flex-direction: column;
font-family: $default-font;
font-size: $default-font-size;
line-height: $default-line-height;
z-index: z-index( '.editor-format-toolbar__link-modal' );

.edit-post-header-toolbar__block-toolbar & {
position: absolute;
left: 0;
right: 0;
}
}

.editor-format-toolbar__link-container {
position: absolute;
transform: translateX( -50% );
z-index: z-index( '.editor-format-toolbar__link-container' );
}

.editor-format-toolbar__link-modal-line {
display: flex;
flex-direction: row;
flex-grow: 1;
flex-shrink: 1;
min-width: 0;
align-items: flex-start;

Expand All @@ -40,6 +18,10 @@
width: $icon-button-size;
height: $icon-button-size;
}

.editor-url-input {
flex-grow: 1;
}
}

.editor-format-toolbar__link-settings-toggle .dashicon {
Expand Down
5 changes: 2 additions & 3 deletions editor/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,11 +446,10 @@ export class RichText extends Component {
getFocusPosition( position ) {
// The container is relatively positioned.
const containerPosition = this.containerRef.current.getBoundingClientRect();
const toolbarOffset = { top: 10, left: 0 };

return {
top: position.top - containerPosition.top + ( position.height ) + toolbarOffset.top,
left: position.left - containerPosition.left + ( position.width / 2 ) + toolbarOffset.left,
top: position.top - containerPosition.top + position.height,
left: position.left - containerPosition.left + ( position.width / 2 ),
};
}

Expand Down
7 changes: 4 additions & 3 deletions editor/components/url-input/button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,10 @@ class UrlInputButton extends Component {
/>
{ expanded &&
<form
className="editor-format-toolbar__link-modal"
onSubmit={ this.submitLink }>
<div className="editor-format-toolbar__link-modal-line">
className="editor-url-input__button-modal"
onSubmit={ this.submitLink }
>
<div className="editor-url-input__button-modal-line">
<IconButton
className="editor-url-input__back"
icon="arrow-left-alt"
Expand Down
Loading