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

Heading block: move alignment controls to toolbar #16682

Closed
wants to merge 7 commits into from
Closed

Heading block: move alignment controls to toolbar #16682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
14204f1
heading controls update
draganescu Aug 11, 2019
308d8ed
removed useless HoC from heading toolbar since we manually determine …
draganescu Aug 11, 2019
fdce1d0
removed useless function body and some useless export
draganescu Aug 15, 2019
f5fc1e8
new snaptshot
draganescu Aug 18, 2019
d9895ae
default collapsed
draganescu Sep 4, 2019
cb01d3f
Revert "default collapsed"
draganescu Sep 4, 2019
060beee
default collapsed
draganescu Sep 4, 2019
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
13 changes: 4 additions & 9 deletions packages/block-library/src/heading/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,19 +62,14 @@ function HeadingEdit( {
<>
<BlockControls>
<HeadingToolbar minLevel={ 2 } maxLevel={ 5 } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
<AlignmentToolbar value={ align } onChange={ ( nextAlign ) => {
setAttributes( { align: nextAlign } );
} } />
</BlockControls>
<InspectorControls>
<PanelBody title={ __( 'Heading Settings' ) }>
<p>{ __( 'Level' ) }</p>
<HeadingToolbar minLevel={ 1 } maxLevel={ 7 } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
draganescu marked this conversation as resolved.
Show resolved Hide resolved
<p>{ __( 'Text Alignment' ) }</p>
<AlignmentToolbar
isCollapsed={ false }
value={ align }
onChange={ ( nextAlign ) => {
setAttributes( { align: nextAlign } );
} }
/>
<HeadingToolbar isCollapsed={ false } minLevel={ 1 } maxLevel={ 7 } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
</PanelBody>
<HeadingColorUI
setTextColor={ setTextColor }
Expand Down
10 changes: 8 additions & 2 deletions packages/block-library/src/heading/heading-toolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,15 @@ class HeadingToolbar extends Component {
}

render() {
const { minLevel, maxLevel, selectedLevel, onChange } = this.props;
const { isCollapsed = true, minLevel, maxLevel, selectedLevel, onChange } = this.props;

return (
<Toolbar controls={ range( minLevel, maxLevel ).map( ( index ) => this.createLevelControl( index, selectedLevel, onChange ) ) } />
<Toolbar
isCollapsed={ isCollapsed }
icon={ 'heading' }
controls={ range( minLevel, maxLevel ).map(
( index ) => this.createLevelControl( index, selectedLevel, onChange )
) } />
);
}
}
Expand Down
11 changes: 10 additions & 1 deletion packages/components/src/dropdown-menu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { flatMap, isEmpty, isFunction } from 'lodash';
import { find, flatMap, isEmpty, isFunction } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -43,6 +43,13 @@ function DropdownMenu( {
}
}

let activeControl = false;
flatMap( controlSets, ( controlSet ) => {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like a very specific implementation of the heading dropdown. Can you think of an alternative approach where DropdownMenu stays unmodified so we could leave this component as is? Ideally, all those CSS styles are applied directly to HeadingToolbar components or alternatively, we modify SVG icons to take the number as param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but that would complicate this PR. I tried to follow the same pattern in the interest of pushing the feature forward. We do already use isActive in the DropdownMenu so I don't feel I've introduced specific Heading related behavior, just some logic to determine the active DropdownMenu element.

I will look into implementing the heading icons differently and get rid of the subscript in the heading component but we'll see. I wonder if there is any benefit of still using subscript for these kinds of icons?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you think of other use cases for subscripts like these?

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be using flatMap if we don't need the return value.

activeControl = find( controlSet, ( control ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we break after finding the active control?

return control.isActive === true;
} );
} );

return (
<Dropdown
className={ classnames( 'components-dropdown-menu', className ) }
Expand All @@ -67,6 +74,7 @@ function DropdownMenu( {
onKeyDown={ openOnArrowDown }
aria-haspopup="true"
aria-expanded={ isOpen }
data-subscript={ activeControl ? activeControl.subscript : '' }
label={ label }
labelPosition={ __unstableLabelPosition }
tooltip={ label }
Expand Down Expand Up @@ -106,6 +114,7 @@ function DropdownMenu( {
},
) }
icon={ control.icon }
data-subscript={ control.subscript }
role="menuitem"
disabled={ control.isDisabled }
>
Expand Down
56 changes: 50 additions & 6 deletions packages/components/src/dropdown-menu/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,21 @@
.components-dropdown-menu__indicator::after {
@include dropdown-arrow();
}

// Subscript for numbered icon buttons, like headings
&[data-subscript]::after {
content: attr(data-subscript);
font-family: $default-font;
font-size: $default-font-size;
font-weight: 600;
position: absolute;
left: 20px;
bottom: 4px;
}

&:not(:disabled).is-active[data-subscript]::after {
color: $white;
}
}
}

Expand All @@ -47,6 +62,7 @@
font-family: $default-font;
font-size: $default-font-size;
line-height: $default-line-height;
position: relative;

.components-dropdown-menu__menu-item,
.components-menu-item {
Expand All @@ -55,6 +71,7 @@
outline: none;
cursor: pointer;
margin-bottom: $grid-size-small;
position: relative;

&.has-separator {
margin-top: 6px;
Expand All @@ -80,19 +97,46 @@
}

// Formatting buttons
> svg {
& > svg {
padding: 5px;
border-radius: $radius-round-rectangle;
height: 30px;
width: 30px;
}

&[data-subscript] svg {
padding: 5px 10px 5px 0;
}

// This assumes 20x20px dashicons.
padding: 2px;
width: $icon-button-size-small;
height: $icon-button-size-small;
margin: -1px $grid-size -1px 0;
// Subscript for numbered icon buttons, like headings
&[data-subscript]::after {
content: attr(data-subscript);
font-family: $default-font;
font-size: $default-font-size;
font-weight: 600;
position: absolute;
left: 23px;
bottom: 11px;
}

&:not(:disabled).is-active[data-subscript]::after {
color: $white;
}

&:not(:disabled).is-active > svg,
&:not(:disabled):hover > svg {
@include formatting-button-style__hover;
}

// Active & toggled style
&:not(:disabled).is-active > svg {
@include formatting-button-style__active;
}

&:not(:disabled):not([aria-disabled="true"]):not(.is-default).is-active > svg {
@include formatting-button-style__active();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this new line snuck in.

}

.components-menu-group:not(:last-child) {
Expand Down
3 changes: 3 additions & 0 deletions packages/edit-post/src/components/header/more-menu/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ exports[`MoreMenu should match snapshot 1`] = `
aria-expanded={false}
aria-haspopup="true"
className="components-dropdown-menu__toggle"
data-subscript=""
icon="ellipsis"
label="More tools & options"
labelPosition="bottom"
Expand All @@ -40,6 +41,7 @@ exports[`MoreMenu should match snapshot 1`] = `
aria-haspopup="true"
aria-label="More tools & options"
className="components-icon-button components-dropdown-menu__toggle"
data-subscript=""
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
Expand All @@ -53,6 +55,7 @@ exports[`MoreMenu should match snapshot 1`] = `
aria-haspopup="true"
aria-label="More tools & options"
className="components-button components-icon-button components-dropdown-menu__toggle"
data-subscript=""
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
Expand Down