Skip to content

Conversation

kevin940726
Copy link
Member

@kevin940726 kevin940726 commented Aug 23, 2022

What?

Fix openDocumentSettingsSidebar util not opening the sidebar.

Why?

Hopefully fixes #34832.

How?

The original implementation relies on toBeVisible() of the role=region[name="Editor settings"i] selector. But it'll always return true even if the settings panel isn't opened.

This PR changes the implementation to look for aria-expanded instead.

Testing Instructions

CI should pass.

@kevin940726 kevin940726 added [Type] Bug An existing feature does not function as intended [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Aug 23, 2022
@talldan
Copy link
Contributor

talldan commented Aug 23, 2022

This PR takes a different approach and calls the internal API instead to open the sidebar.

I think this is ok, but my concern is it would reduce coverage for an essential part of the user interface.

Should some tests be added for the toggle button, or are there some already?

@kevin940726
Copy link
Member Author

Should some tests be added for the toggle button, or are there some already?

Yeah, ideally we should add back some tests for the toggle button. I'm not sure if there's one already.

@kevin940726 kevin940726 force-pushed the fix/open-document-settings-sidebar branch from 4ff960b to 8563355 Compare August 24, 2022 03:22
@kevin940726 kevin940726 force-pushed the fix/open-document-settings-sidebar branch from 8563355 to 860db94 Compare August 24, 2022 07:03
@kevin940726
Copy link
Member Author

Since the internal API approach only works in the post editor, I changed the implementation to using aria-expanded instead.

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for iterating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flaky Test] should replace, reset size, and keep selection
2 participants