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

Fix dark menus in canvas, not in top toolbar #22516

Merged
merged 4 commits into from
Jun 8, 2020
Merged

Fix dark menus in canvas, not in top toolbar #22516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3fcdf2d
Fix matrix popover style.
jasmussen May 21, 2020
8488f8e
Fix for replace flow.
jasmussen May 21, 2020
5b1668c
Fix alignment toolbars.
jasmussen May 21, 2020
cfb7cef
Update snapshots.
jasmussen May 25, 2020
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
3 changes: 2 additions & 1 deletion packages/block-editor/src/components/block-alignment-matrix-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,13 @@ export function BlockAlignmentMatrixToolbar( props ) {
const icon = <AlignmentMatrixControl.Icon value={ value } />;
const className = 'block-editor-block-alignment-matrix-toolbar';
const popoverClassName = `${ className }__popover`;
const isAlternate = true;

return (
<Dropdown
position="bottom right"
className={ className }
popoverProps={ { className: popoverClassName } }
popoverProps={ { className: popoverClassName, isAlternate } }
renderToggle={ ( { onToggle, isOpen } ) => {
const openOnArrowDown = ( event ) => {
if ( ! isOpen && event.keyCode === DOWN ) {
Expand Down
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/block-alignment-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ const DEFAULT_CONTROLS = [ 'left', 'center', 'right', 'wide', 'full' ];
const DEFAULT_CONTROL = 'center';
const WIDE_CONTROLS = [ 'wide', 'full' ];

const POPOVER_PROPS = {
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of having this as a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the only way I found to get this prop to take.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the advantage is that the object is not regenerated on each render. On expensive pure components, it could have an impact but maybe not everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to do which ever you agree on. Just haven't been able to get it to work without this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to unblock this PR, so we don't ship the current change in master if we can avoid it. CC: @youknowriad

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasmussen this shouldn't be a blocker IMO. You can inline these if needed. Performance is probably not a concern here.

isAlternate: true,
};

export function BlockAlignmentToolbar( {
value,
onChange,
Expand All @@ -68,6 +72,7 @@ export function BlockAlignmentToolbar( {

return (
<ToolbarGroup
popoverProps={ POPOVER_PROPS }
isCollapsed={ isCollapsed }
icon={
activeAlignmentControl
Expand Down
5 changes: 5 additions & 0 deletions ...ages/block-editor/src/components/block-alignment-toolbar/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,10 @@ exports[`BlockAlignmentToolbar should match snapshot 1`] = `
}
isCollapsed={true}
label="Change alignment"
popoverProps={
Object {
"isAlternate": true,
}
}
/>
`;
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/block-vertical-alignment-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ const BLOCK_ALIGNMENTS_CONTROLS = {
const DEFAULT_CONTROLS = [ 'top', 'center', 'bottom' ];
const DEFAULT_CONTROL = 'top';

const POPOVER_PROPS = {
isAlternate: true,
};

export function BlockVerticalAlignmentToolbar( {
value,
onChange,
Expand All @@ -49,6 +53,7 @@ export function BlockVerticalAlignmentToolbar( {

return (
<ToolbarGroup
popoverProps={ POPOVER_PROPS }
isCollapsed={ isCollapsed }
icon={
activeAlignment
Expand Down
5 changes: 5 additions & 0 deletions ...k-editor/src/components/block-vertical-alignment-toolbar/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,10 @@ exports[`BlockVerticalAlignmentToolbar should match snapshot 1`] = `
}
isCollapsed={true}
label="Change vertical alignment"
popoverProps={
Object {
"isAlternate": true,
}
}
/>
`;
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/media-replace-flow/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,13 @@ const MediaReplaceFlow = ( {
}
};

const POPOVER_PROPS = {
isAlternate: true,
};

return (
<Dropdown
popoverProps={ POPOVER_PROPS }
contentClassName="block-editor-media-replace-flow__options"
renderToggle={ ( { isOpen, onToggle } ) => (
<ToolbarGroup className="media-replace-flow">
Expand Down
1 change: 0 additions & 1 deletion packages/components/src/dropdown/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ class Dropdown extends Component {
expandOnMobile={ expandOnMobile }
headerTitle={ headerTitle }
focusOnMount={ focusOnMount }
isAlternate
{ ...popoverProps }
className={ classnames(
'components-dropdown__content',
Expand Down