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.

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

E2E: add test to check admin bar presence in mobile. #57579

Merged
merged 6 commits into from
Nov 4, 2021

Conversation

worldomonation
Copy link
Contributor

@worldomonation worldomonation commented Nov 2, 2021

Changes proposed in this Pull Request

This PR adds a test as per #57190 to check the presence and basic functionality of the admin bar (also called navbar).

Key changes:

  • change name of some existing selectors.
  • specify :visible pseudo-CSS.
  • add handling for different mobile/desktop behavior for exiting the editor.

Details:
Changes in this PR arose from the parent issue #57190 and #51162 where on the mobile web editor no methods were provided for the user to exit the editor back to WordPress.com.

The changes in #57234 were to persistently show the navbar in the editor view, so the user can click on My Sites to return to WordPress.com.

However, this means that experience of exiting the editor differs depending on the viewport size used:

  • desktop: click on the site icon, then click on 'Dashboard'.
  • mobile: click on My Sites.

The method openNavSidebar has been restructured so that it only performs its action on desktop.
Similarly, returnToHomeDashboard has been restructured to handle both scenarios.

Testing instructions

  • stress test
    • mobile 6924664
    • desktop 6924674
  • normal test
    • mobile
    • desktop

Closes #57190.

@github-actions
Copy link

github-actions bot commented Nov 2, 2021

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@worldomonation worldomonation force-pushed the pw/editor-admin-bar-57190 branch from b747130 to b4ad308 Compare November 2, 2021 22:26
@worldomonation worldomonation self-assigned this Nov 2, 2021
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 3, 2021
@worldomonation worldomonation marked this pull request as ready for review November 3, 2021 18:51
@worldomonation worldomonation requested review from scinos, WunderBart and a team as code owners November 3, 2021 18:51
Copy link
Contributor

@dpasque dpasque left a comment

Choose a reason for hiding this comment

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

Added a few comments! The only one I feel strongly about the calypso-pr group inclusion!

Otherwise looks great - thanks for doing this! 👍

const frame = await this.getEditorFrame();
await frame.click( selectors.dashboardLink );
const navbarComponent = new NavbarComponent( this.page );
const actions: unknown[] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we know the type here, right? It's a Promise<any>[] I believe!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes I was missing the Promise part. Reason why I chose unknown is because the transpiler won't show me a warning that way :P

*/
async returnToDashboard(): Promise< void > {
async returnToHomeDashboard(): Promise< void > {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: but I kind of feel like that function should take care of opening the nav sidebar for desktop if needed. It feels a little weird that people using this function have to include a call to openNavSidebar in their specs before it, but that only matters for the desktop case.

@@ -0,0 +1,37 @@
/**
* @group gutenberg
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 we should run this on calypso-pr runs, right? That navbar is pure calypso, and we override some stuff in the nav sidebar, so I think it's directly relevant to PR runs!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I completely forgot about this one - I copy-pasted the test contents and simply forgot to update it.

- support both mobile and desktop sizes
Add new sped `Editor: Navbar`
- open the sidebar as part of the returning to home dashboard.
- change type of the action array.
@worldomonation worldomonation force-pushed the pw/editor-admin-bar-57190 branch from 650b97b to 0d0c748 Compare November 4, 2021 18:05
Copy link
Contributor

@dpasque dpasque left a comment

Choose a reason for hiding this comment

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

🚢

@worldomonation worldomonation merged commit cefbbcc into trunk Nov 4, 2021
@worldomonation worldomonation deleted the pw/editor-admin-bar-57190 branch November 4, 2021 19:39
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 4, 2021
nelsonec87 pushed a commit that referenced this pull request Dec 9, 2021
* Change how `openNavSidebar` and `returnToHomeDashboard` works.
- support both mobile and desktop sizes

* Update tests

* Use an array of promises intead.
Add new sped `Editor: Navbar`

* Fix lint error.

* Another lint error.

* Minor updates from code review.
- open the sidebar as part of the returning to home dashboard.
- change type of the action array.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E2E: Add test for the admin bar presence when in editor
3 participants