Components: Show popovers fullscreen on mobile #3400

youknowriad · 2017-11-09T10:01:13Z

This PR updates the Popover Component to show all popovers fullscreen on mobile.
This makes the inserter more usable on mobile. This also adds an expandOnMobile prop to enable this behavior for specific popovers. It's only applied to the inserter right now.

Testing instructions

Try the inserter on mobile
Try the inserter on desktop to ensure nothing is broken
Try several popovers.

jorgefilipecosta

Hi @youknowriad, nice work this generally works well 👍 I just noticed some improvements.
I think we need an option to close the popover's for example if we go to the inserter we cannot close it without inserting a block. Another problem is that in some resolutions the ones in the middle of small (600) and medium (782) e.g 678px width, the popover of the block (when you press (...) at the side) just display a big white rectangle and is impossible to use.

codecov · 2017-11-09T10:29:29Z

Codecov Report

Merging #3400 into master will increase coverage by <.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #3400      +/-   ##
==========================================
+ Coverage   36.92%   36.92%   +<.01%     
==========================================
  Files         267      267              
  Lines        6662     6675      +13     
  Branches     1203     1205       +2     
==========================================
+ Hits         2460     2465       +5     
- Misses       3550     3557       +7     
- Partials      652      653       +1

Impacted Files	Coverage Δ
components/tab-panel/index.js	`93.75% <ø> (ø)`	⬆️
editor/components/inserter/index.js	`0% <ø> (ø)`	⬆️
editor/components/inserter/menu.js	`85.54% <100%> (ø)`	⬆️
components/dropdown/index.js	`70.83% <100%> (ø)`	⬆️
components/popover/index.js	`80.5% <42.85%> (-5.21%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3399b4c...6ebaa82. Read the comment docs.

jorgefilipecosta · 2017-11-09T10:28:30Z

components/popover/style.scss

 		bottom: 100%;
 	}

-	.components-popover.is-center & {
+	.components-popover:not(.is-mobile).is-center & {


Instead of using a series of :not(.is-mobile) and given that is the component itself that sets the is-mobile depending on a width. I think using a CSS rule that uses our variable $break-medium may be an option to simplify this.

is-mobile can be disabled even on mobile because this behavior is opt-out. Relying on the $break-medium would apply this consistently.

jorgefilipecosta · 2017-11-09T10:34:22Z

components/popover/index.js

@@ -35,6 +35,7 @@ const ARROW_OFFSET = 20;
 * @type {String}
 */
 const SLOT_NAME = 'Popover';
+const isMobile = () => window.innerWidth < 782;


This is one more place to sync with our scss variables. I think for now it is ok but this logic could benefit from #3331 where we add a synchronization with our scss variables, and from #3332 where we add responsive-redux making is-mobile being a prop the component receives.

The problem is, the Popover is a generic component outside the editor module. I thought about factorizing this but we can't assume any context in the components module.

Right, I would say given this it would be better to simplify the component and just receive a showFullScreen and than is up the callers to decide if is mobile and the enable this option (maybe using responsive redux) or they don't enable this option. The downside is we would need to activate this option manually in the cases we want.

That's an option but I worry about having the component's user compute the isMobile on its own. I think this would create more duplication than it's supposed to avoid especially for third-party code or WP code no in the editor module.

I personally don't mind the small duplication here to make it generic enough.

Yes, that is true it would require more effort from callers. In that case, another option would be instead of expandOnMobile we receive a prop showFullScreen that defaults to our isMobile function (If no value is passed). The advantage is that callers are able to use their own logic and their own breakpoint for mobile.

jasmussen · 2017-11-09T10:51:37Z

Thanks for working on this. I think it may be going slightly too far. I do appreciate what modality can do for us with regards to a teensy screen that might have most of it covered by browser chrome and soft keyboards. But I don't know that we can blanket decide that all popovers have to cover everything. Is it possible to make this an "opt-in" feature on the popover component?

Also, we'll definitely need a dismiss button on every modal popover, so there's a way to exit it without the escape key.

youknowriad · 2017-11-09T11:07:28Z

Is it possible to make this an "opt-in" feature on the popover component?

Yes, I asked this question above in the description :)

Also, we'll definitely need a dismiss button on every modal popover, so there's a way to exit it without the escape key.

I can try to add a close button on all "fullscreen" popovers. This could create some styling issues though because it will cover a small area at the top right of the popover.

youknowriad · 2017-11-09T11:25:47Z

Updated this, it's opt-in and only applied on the Inserter for now.

jasmussen · 2017-11-09T12:11:30Z

Nice, the opt-in change is cool.

Since this is very much a mobile property, I think we should output the X to close no matter the fact that it'll add styling issues. These are issues we should track and fix — and yes, the mobile inserter is on my todo list. But as it stands, you can't dismiss the modal on mobile without a X so it should be considered a requirement. Okay with styling issues, in other words.

By the way, the Settings sidebar is also modal, and with a X button. I think it's modal through CSS only — should the sidebar be changed to use this method to go modal also? Given our sidebar idea in #3330, perhaps that would be nice?

youknowriad · 2017-11-09T13:03:08Z

By the way, the Settings sidebar is also modal, and with a X button. I think it's modal through CSS only — should the sidebar be changed to use this method to go modal also? Given our sidebar idea in #3330, perhaps that would be nice?

I think to achieve #3330's sidebar, we need a generic sidebar component, which we do not have yet, and we could replicate the popover's 2 fullscreen behavior in this component as well.

aduth · 2017-11-09T20:49:40Z

components/popover/test/index.js

@@ -227,6 +227,8 @@ describe( 'Popover', () => {
 				style: {},
 			};

+			instance.setState = () => {};


If we assigned this as jest.fn() we could add some tests to confirm called with expected behaviors 😄

aduth · 2017-11-09T20:50:25Z

components/popover/index.js

+			return;
+		}
+		this.setState( {
+			isMobile: false,


This will always trigger a re-render, even if this.state.isMobile is already false.

aduth · 2017-11-09T20:50:31Z

components/popover/index.js

@@ -169,6 +185,9 @@ class Popover extends Component {
 			return;
 		}

+		//popover.style.bottom = 'auto';


Remove commented code.

aduth · 2017-11-09T20:51:13Z

components/popover/index.js

@@ -258,6 +278,9 @@ class Popover extends Component {
 			className,
 			'is-' + yAxis,
 			'is-' + xAxis,
+			{
+				'is-mobile': this.state.isMobile,


Does this need to be state? Can't we just call isMobile()?

yes, because we need to rerender if it changes.

youknowriad · 2017-11-13T09:06:28Z

Added a close button automatically on mobile.

jasmussen · 2017-11-13T10:34:57Z

Nice work. I think this gets us within spitting range of something great. I feel like we should take the next step and just add a top bar and a label when fullscreen modal is invoked like this.

See the sidebar:

Perhaps we could add something similar to the inserter — and indeed anything that invokes as a modal:

That's some quick and dirty inspector surgery. We could add a label on the left, like "Add" or "Add block".

Let me know if you'd like me to help out here, I can commit to this branch.

youknowriad · 2017-11-13T10:39:21Z

@jasmussen Nice design, Feel free to commit if you have bandwidth. Otherwise, I can update it later.

youknowriad · 2017-11-14T10:07:12Z

PR updated with the header

jasmussen · 2017-11-14T10:37:20Z

Nice work! Sorry I didn't get to it.

There's a double border in the header compared to the settings box:

youknowriad · 2017-11-21T09:35:24Z

This should be ready to go

jorgefilipecosta · 2017-11-21T10:59:22Z

Hi @youknowriad, I did some tests and it looks like, there is a regression. In my environment the last two blocks on the inserter. Get out of the pop-over area.

youknowriad · 2017-11-21T11:39:27Z

@jorgefilipecosta Good catch, looks like a regression introduced by the TabPanel refactoring (when rebasing)

youknowriad · 2017-11-21T12:35:06Z

@jorgefilipecosta Hopefully, it should be fixed with the last commit

youknowriad self-assigned this Nov 9, 2017

youknowriad requested review from jasmussen and aduth November 9, 2017 10:01

youknowriad force-pushed the fix/fullscreen-mobile-popover branch from 5ab5a3c to e6c80bf Compare November 9, 2017 10:16

jorgefilipecosta reviewed Nov 9, 2017

View reviewed changes

youknowriad force-pushed the fix/fullscreen-mobile-popover branch from cdc4569 to ac0112f Compare November 9, 2017 12:03

aduth reviewed Nov 9, 2017

View reviewed changes

youknowriad force-pushed the fix/fullscreen-mobile-popover branch from 2ea3a81 to 9b7cef0 Compare November 13, 2017 09:04

youknowriad force-pushed the fix/fullscreen-mobile-popover branch 4 times, most recently from 400d11e to ded6edd Compare November 21, 2017 09:34

youknowriad added 2 commits November 22, 2017 09:31

Components: Show popovers fullscreen on mobile

4defb1d

Popover: Add expandOnMobile prop

1e30431

youknowriad added 7 commits November 22, 2017 09:31

Popovers: Fix Tooltip Width

a52de4f

Popovere: Fix unit tests

d0e6c6a

Popover: Making the expandOnMobile prop opt-in

8fa3cf5

Popovers: Automatically add a close button for mobile popoveres

5960fa3

Popover: Avoid resetting the isMobile state when not necessary

063ea58

Popover: Adding a header for mobile popovers

151066f

Inserter: Fix Inserter styles broken after rebase

6ebaa82

youknowriad force-pushed the fix/fullscreen-mobile-popover branch from f510287 to 6ebaa82 Compare November 22, 2017 08:32

youknowriad merged commit 2bb48d5 into master Nov 22, 2017

youknowriad deleted the fix/fullscreen-mobile-popover branch November 22, 2017 08:56

aduth mentioned this pull request May 6, 2019

Focus not visible on Content Structure pane #15324

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Components: Show popovers fullscreen on mobile #3400

Components: Show popovers fullscreen on mobile #3400

youknowriad commented Nov 9, 2017 •

edited

Loading

jorgefilipecosta left a comment •

edited

Loading

codecov bot commented Nov 9, 2017 •

edited

Loading

jorgefilipecosta Nov 9, 2017

youknowriad Nov 9, 2017 •

edited

Loading

jorgefilipecosta Nov 9, 2017

youknowriad Nov 9, 2017

jorgefilipecosta Nov 9, 2017 •

edited

Loading

youknowriad Nov 9, 2017

jorgefilipecosta Nov 9, 2017

jasmussen commented Nov 9, 2017

youknowriad commented Nov 9, 2017

youknowriad commented Nov 9, 2017

jasmussen commented Nov 9, 2017

youknowriad commented Nov 9, 2017

aduth Nov 9, 2017

aduth Nov 9, 2017

aduth Nov 9, 2017

aduth Nov 9, 2017

youknowriad Nov 13, 2017

youknowriad commented Nov 13, 2017

jasmussen commented Nov 13, 2017

youknowriad commented Nov 13, 2017

youknowriad commented Nov 14, 2017

jasmussen commented Nov 14, 2017

youknowriad commented Nov 21, 2017

jorgefilipecosta commented Nov 21, 2017

youknowriad commented Nov 21, 2017

youknowriad commented Nov 21, 2017

Components: Show popovers fullscreen on mobile #3400

Components: Show popovers fullscreen on mobile #3400

Conversation

youknowriad commented Nov 9, 2017 • edited Loading

jorgefilipecosta left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 9, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

youknowriad Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Nov 9, 2017

youknowriad commented Nov 9, 2017

youknowriad commented Nov 9, 2017

jasmussen commented Nov 9, 2017

youknowriad commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Nov 13, 2017

jasmussen commented Nov 13, 2017

youknowriad commented Nov 13, 2017

youknowriad commented Nov 14, 2017

jasmussen commented Nov 14, 2017

youknowriad commented Nov 21, 2017

jorgefilipecosta commented Nov 21, 2017

youknowriad commented Nov 21, 2017

youknowriad commented Nov 21, 2017

youknowriad commented Nov 9, 2017 •

edited

Loading

jorgefilipecosta left a comment •

edited

Loading

codecov bot commented Nov 9, 2017 •

edited

Loading

youknowriad Nov 9, 2017 •

edited

Loading

jorgefilipecosta Nov 9, 2017 •

edited

Loading