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

Mobile Web: Ensure that sidebar is closed on the first visit #17902

Merged
merged 2 commits into from
Oct 11, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Oct 11, 2019

Description

Related to #17712.

Fixes #17341.
Fixes #17708.

Describe the bug

I was going to test gutenberg on a new install (via npm run env install) with the iOS simulator, and noticed this is what you see the first time you open gutenberg:

gutenberg-ios-nux

First, instead of showing the editor, the settings sidebar is initially visible, completely covering the editor.

I still need to add a proper fix, for the time being, I updated the test to catch this bug.

Steps to reproduce

  1. Clear the local storage
  2. Set a mobile viewport in your browser when using Desktop or use any mobile device with a smaller screen.
  3. Open Add New Post page.

With the fix applied Sidebar shouldn't cover the screen, it should be closed by default.

How has this been tested?

I updated e2e tests to ensure they catch this issue in the future in 4c68cb2.
They should all pas with:
npm run test-e2e

@gziolo gziolo added [Type] Bug An existing feature does not function as intended Mobile Web Viewport sizes for mobile and tablet devices [Type] Regression Related to a regression in the latest release labels Oct 11, 2019
@gziolo gziolo self-assigned this Oct 11, 2019
@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Oct 11, 2019
@gziolo
Copy link
Member Author

gziolo commented Oct 11, 2019

The updated e2e test fails as expected without the production code updated. See https://travis-ci.com/WordPress/gutenberg/jobs/244637978#L1025:

 ● Sidebar › should have the sidebar closed by default on mobile
    expect(received).toBeNull()
    Received: {"_client": {"_callbacks": [Map], "_connection": [Connection], "_events": [Object], "_eventsCount": 28, "_maxListeners": undefined, "_sessionId": "CF5F75128702B39AE7F47D868B9C4E7C", "_targetType": "page"}, "_context": {"_client": [CDPSession], "_contextId": 9, "_world": [DOMWorld]}, "_disposed": false, "_frameManager": {"_client": [CDPSession], "_contextIdToContext": [Map], "_events": [Object], "_eventsCount": 3, "_frames": [Map], "_isolatedWorlds": [Set], "_mainFrame": [Frame], "_maxListeners": undefined, "_networkManager": [NetworkManager], "_page": [Page], "_timeoutSettings": [TimeoutSettings]}, "_page": {"_accessibility": [Accessibility], "_client": [CDPSession], "_closed": false, "_coverage": [Coverage], "_emulationManager": [EmulationManager], "_events": [Object], "_eventsCount": 5, "_fileChooserInterceptionIsDisabled": false, "_fileChooserInterceptors": [Set], "_frameManager": [FrameManager], "_javascriptEnabled": true, "_keyboard": [Keyboard], "_maxListeners": undefined, "_mouse": [Mouse], "_pageBindings": [Map], "_screenshotTaskQueue": [TaskQueue], "_target": [Target], "_timeoutSettings": [TimeoutSettings], "_touchscreen": [Touchscreen], "_tracing": [Tracing], "_viewport": [Object], "_workers": [Map]}, "_remoteObject": {"className": "HTMLDivElement", "description": "div.edit-post-sidebar", "objectId": "{\"injectedScriptId\":9,\"id\":3}", "subtype": "node", "type": "object"}}
      51 | 		await createNewPost();
      52 | 		const sidebar = await page.$( SIDEBAR_SELECTOR );
    > 53 | 		expect( sidebar ).toBeNull();
         | 		                  ^
      54 | 	} );
      55 | 
      56 | 	it( 'should close the sidebar when resizing from desktop to mobile', async () => {
      at Object.toBeNull (specs/sidebar.test.js:53:21)

@gziolo
Copy link
Member Author

gziolo commented Oct 11, 2019

e78d12d should resolve the issue. @nerrad - I figured out this is also related to the changes applied in #17712 so I would appreciate your review.

We should ensure sidebar plugins do not get auto-closed when opened on smaller screens. I tested it myself and it works fine. We also have e2e tests that cover that.

@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Oct 11, 2019
@gziolo gziolo requested review from koke and youknowriad October 11, 2019 10:36
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This fixes the issue for me.

@gziolo gziolo merged commit ecb6ab1 into master Oct 11, 2019
@gziolo gziolo deleted the fix/17708-sidebar-closed-on-first-visit-mobile branch October 11, 2019 11:15
@nerrad
Copy link
Contributor

nerrad commented Oct 11, 2019

Ack! Sorry for not catching this in my review of #17712 😓

@youknowriad youknowriad added this to the Gutenberg 6.7 milestone Oct 14, 2019
gziolo added a commit that referenced this pull request Oct 15, 2019
* Mobile Web: Ensure that sidebar is closed on the first visit

* Mobile Web: Ensure that sidebar is closed on the first visit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile Web Viewport sizes for mobile and tablet devices [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

First launch on mobile is unwelcoming Document/Block Sidebar Covering Content on First Post Visit on Mobile
3 participants