Skip to content

Block Editor: Wait for popover positioning in MediaReplaceFlow tests #45863

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

Merged
merged 10 commits into from
Nov 21, 2022
Merged

Block Editor: Wait for popover positioning in MediaReplaceFlow tests #45863

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
001d6b6
Block Editor: Wait for popover positioning in MediaReplaceFlow tests
tyxla Nov 17, 2022
e035d52
Remove an extra backtick
tyxla Nov 18, 2022
1f7ee28
Use Element.closest() to locate closest popover
tyxla Nov 18, 2022
99069f9
Fix comment
tyxla Nov 18, 2022
1e65537
Remove an excess assertion
tyxla Nov 18, 2022
8032ab0
Reorder assertions
tyxla Nov 18, 2022
e59733d
Introduce a less-specific way to assert positioning
tyxla Nov 18, 2022
85d318d
Use the right element when locating the popover
tyxla Nov 18, 2022
6c63bc5
Clarify element.style usage
tyxla Nov 18, 2022
56eef5d
Wait for the menu to get visible
tyxla Nov 18, 2022
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
Prev Previous commit
Next Next commit
Use Element.closest() to locate closest popover
  • Loading branch information
@tyxla
tyxla committed Nov 18, 2022
commit 1f7ee28d5606de43257516e6312ecd7b5edb4d8d
5 changes: 1 addition & 4 deletions packages/block-editor/src/components/media-replace-flow/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,7 @@ function TestWrapper() {
* @return {HTMLElement|null} Popover element, or `null` if not found.
*/
function getWrappingPopoverElement( element ) {
if ( element.classList.contains( 'components-popover' ) ) {
return element;
}
return getWrappingPopoverElement( element.parentElement );
return element.closest( '.components-popover' );
Copy link
Member Author

@tyxla tyxla Nov 18, 2022

Choose a reason for hiding this comment

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

@jsnajdr a note on this approach: when we have direct access to the Dropdown props, we can pass a data-testid prop through popoverProps and it will be a more optimal way to access the popover than using .closest(). I've used this technique in #45911 if you're interested to see an example.

Copy link
Contributor

@ciampo ciampo Nov 18, 2022

Choose a reason for hiding this comment

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

Agreed, using data-test-id sounds like a better option when possible.

Do we ever add data-testid to a component's source, or does that only happen inside the tests ?

Copy link
Member Author

@tyxla tyxla Nov 18, 2022

Choose a reason for hiding this comment

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

I'd never recommend cluttering the component source, but for test fixtures, I've found it to be nice and clean as a last resort.

Copy link
Member

@jsnajdr jsnajdr Nov 19, 2022

Choose a reason for hiding this comment

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

I'm afraid I don't get the point of data-testid -- on what scale is it a more optimal way to access the popover? That it doesn't trigger the no-node-access rule?

Copy link
Member Author

@tyxla tyxla Nov 19, 2022

Choose a reason for hiding this comment

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

That, too, but there is more. It also allows us to avoid testing implementation details, and having to peek into the implementation of other components. That makes our tests more resilient since they don't rely on the implementation details of other components we're not currently testing.

Copy link
Member Author

@tyxla tyxla Nov 21, 2022

Choose a reason for hiding this comment

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

FWIW I just wanted to point it out as a potential alternative; we can't achieve it here without altering the component source because popoverProps don't accept additional props.

Copy link
Member

@jsnajdr jsnajdr Nov 21, 2022

Choose a reason for hiding this comment

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

I think it'll be hard to avoid testing implementation details: the way how the popover is positioned, which element and with what CSS styles, that it's done async -- these are all implementation details. The best we can do is to wrap them inside helpers like custom queries or matchers.

Even the fact that the positioned div and the div that gets the data-testid prop are the same element is an implementation detail. It doesn't always need to be so. The rest props are internally called contentProps, so maybe they were applied to components-popover_content element in the past?

Also, as you write, it's not always possible to pass popoverProps, especially when testing a larger unit, like a full block edit UI, where there are multiple tooltips and menus.

Copy link
Member Author

@tyxla tyxla Nov 21, 2022

Choose a reason for hiding this comment

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

That's fair, and there's not much we can do to make things easier for composable components that offer larger flexibility through a plethora of props.

Do you have any reservations about the approach proposed in the PR after the changes I made @jsnajdr ?

}

describe( 'General media replace flow', () => {
Expand Down