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

Components: Show popovers fullscreen on mobile #3400

Merged
merged 9 commits into from Nov 22, 2017

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented Nov 9, 2017

refs #2691

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.

@youknowriad youknowriad self-assigned this Nov 9, 2017

@youknowriad youknowriad requested review from jasmussen and aduth Nov 9, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 9, 2017

Codecov Report

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

Impacted file tree graph

@@            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.

codecov bot commented Nov 9, 2017

Codecov Report

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

Impacted file tree graph

@@            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.

bottom: 100%;
}
.components-popover.is-center & {
.components-popover:not(.is-mobile).is-center & {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 9, 2017

Member

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.

@jorgefilipecosta

jorgefilipecosta Nov 9, 2017

Member

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.

This comment has been minimized.

@youknowriad

youknowriad Nov 9, 2017

Contributor

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

@youknowriad

youknowriad Nov 9, 2017

Contributor

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

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

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 9, 2017

Member

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.

@jorgefilipecosta

jorgefilipecosta Nov 9, 2017

Member

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.

This comment has been minimized.

@youknowriad

youknowriad Nov 9, 2017

Contributor

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.

@youknowriad

youknowriad Nov 9, 2017

Contributor

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.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 9, 2017

Member

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.

@jorgefilipecosta

jorgefilipecosta Nov 9, 2017

Member

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.

This comment has been minimized.

@youknowriad

youknowriad Nov 9, 2017

Contributor

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.

@youknowriad

youknowriad Nov 9, 2017

Contributor

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.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Nov 9, 2017

Member

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.

@jorgefilipecosta

jorgefilipecosta Nov 9, 2017

Member

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

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Nov 9, 2017

Contributor

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.

screen shot 2017-11-09 at 11 49 55

Contributor

jasmussen commented Nov 9, 2017

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.

screen shot 2017-11-09 at 11 49 55

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 9, 2017

Contributor

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.

Contributor

youknowriad commented Nov 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 9, 2017

Contributor

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

Contributor

youknowriad commented Nov 9, 2017

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

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Nov 9, 2017

Contributor

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?

Contributor

jasmussen commented Nov 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 9, 2017

Contributor

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.

Contributor

youknowriad commented Nov 9, 2017

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.

Show outdated Hide outdated components/popover/test/index.js Outdated
Show outdated Hide outdated components/popover/index.js Outdated
Show outdated Hide outdated components/popover/index.js Outdated
@@ -258,6 +278,9 @@ class Popover extends Component {
className,
'is-' + yAxis,
'is-' + xAxis,
{
'is-mobile': this.state.isMobile,

This comment has been minimized.

@aduth

aduth Nov 9, 2017

Member

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

@aduth

aduth Nov 9, 2017

Member

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

This comment has been minimized.

@youknowriad

youknowriad Nov 13, 2017

Contributor

yes, because we need to rerender if it changes.

@youknowriad

youknowriad Nov 13, 2017

Contributor

yes, because we need to rerender if it changes.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 13, 2017

Contributor

Added a close button automatically on mobile.

Contributor

youknowriad commented Nov 13, 2017

Added a close button automatically on mobile.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Nov 13, 2017

Contributor

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:

screen shot 2017-11-13 at 11 30 13

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

screen shot 2017-11-13 at 11 32 11

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.

Contributor

jasmussen commented Nov 13, 2017

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:

screen shot 2017-11-13 at 11 30 13

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

screen shot 2017-11-13 at 11 32 11

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

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 13, 2017

Contributor

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

Contributor

youknowriad commented Nov 13, 2017

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

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 14, 2017

Contributor

PR updated with the header

Contributor

youknowriad commented Nov 14, 2017

PR updated with the header

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Nov 14, 2017

Contributor

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

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

screen shot 2017-11-14 at 11 35 18

screen shot 2017-11-14 at 11 35 24

Contributor

jasmussen commented Nov 14, 2017

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

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

screen shot 2017-11-14 at 11 35 18

screen shot 2017-11-14 at 11 35 24

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 21, 2017

Contributor

This should be ready to go

Contributor

youknowriad commented Nov 21, 2017

This should be ready to go

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Nov 21, 2017

Member

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.
screen shot 2017-11-21 at 10 56 05

Member

jorgefilipecosta commented Nov 21, 2017

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.
screen shot 2017-11-21 at 10 56 05

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 21, 2017

Contributor

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

Contributor

youknowriad commented Nov 21, 2017

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

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 21, 2017

Contributor

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

Contributor

youknowriad commented Nov 21, 2017

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

@youknowriad youknowriad merged commit 2bb48d5 into master Nov 22, 2017

3 checks passed

codecov/project 36.92% (+<.01%) compared to 3399b4c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/fullscreen-mobile-popover branch Nov 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment