Skip to content

Try to fix the popover position when changing to show button text labels #43327

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

Closed
ntsekouras wants to merge 1 commit into from
Closed

Try to fix the popover position when changing to show button text labels #43327

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Try to fix the popover position when changing to `show button text la… 
…bels`
  • Loading branch information
@ntsekouras
ntsekouras committed Aug 17, 2022
commit 2fddb1138698003f009e95a8ec8b8dd85c1feeac
3 changes: 3 additions & 0 deletions packages/edit-post/src/components/header/more-menu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
PinnedItems,
} from '@wordpress/interface';
import { useViewportMatch } from '@wordpress/compose';
import { useRef } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -19,6 +20,7 @@ import ToolsMoreMenuGroup from '../tools-more-menu-group';
import WritingMenu from '../writing-menu';

const MoreMenu = ( { showIconLabels } ) => {
const anchorRef = useRef();
Copy link
Member

@ellatrix ellatrix Aug 17, 2022

Choose a reason for hiding this comment

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

Must be missing something. Where does this ref get attached?

Copy link
Contributor

@andrewserong andrewserong Aug 18, 2022

Choose a reason for hiding this comment

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

I was wondering if this one is working specifically because it isn't attached to anything, so it overrides the Popover's default positioning?

Copy link
Contributor

@ciampo ciampo Aug 18, 2022

Choose a reason for hiding this comment

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

Yeah this is unusual, I would also expect this ref to be attached to an element

const isLargeViewport = useViewportMatch( 'large' );

return (
Expand All @@ -27,6 +29,7 @@ const MoreMenu = ( { showIconLabels } ) => {
showTooltip: ! showIconLabels,
...( showIconLabels && { variant: 'tertiary' } ),
} }
popoverProps={ { anchorRef: anchorRef?.current?.firstChild } }
Copy link
Member

@ellatrix ellatrix Aug 17, 2022

Choose a reason for hiding this comment

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

Why do we need the first child?

>
{ ( { onClose } ) => (
<>
Expand Down