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

Always display the mobile nav menu when disconnecting sites #14925

Merged
merged 5 commits into from
Mar 12, 2020

Conversation

enejb
Copy link
Member

@enejb enejb commented Mar 9, 2020

Make the cancel and disconnect buttons always visible on mobile devices.

Fixes #8726
Before:
Screen Shot 2020-03-09 at 11 56 49 AM

After:
Screen Shot 2020-03-09 at 11 55 56 AM

Changes proposed in this Pull Request:

Testing instructions:

  • Connect your site.
  • On a mobile iPhone or any other device (use browserstack.com ) device try disconnection.
  • Are you able to do it? Do you see the button as expected?

Proposed changelog entry for your changes:

  • Fix broken flow. Show the disconnect button on mobile always.

Make the cancel and disconnect buttons always visible on mobile devices.
@enejb enejb added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Needs Testing We need to add this change to the testing call for this month's release Connect Flow Connection banners, buttons, ... [Status] Needs Design labels Mar 9, 2020
@enejb enejb added this to the 8.4 milestone Mar 9, 2020
@enejb enejb requested a review from a team March 9, 2020 10:59
@enejb enejb self-assigned this Mar 9, 2020
@jetpackbot
Copy link

jetpackbot commented Mar 9, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against b150c3f

Copy link
Contributor

@keoshi keoshi left a comment

Choose a reason for hiding this comment

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

In general this looks good to me and it's a great change.

Added a comment about a color variable below, which is minor, but I noticed we're not showing the modal's title if the device is shorter than the content. See below:

image

This affects all sorts of devices, from iPhone SE/5/6/7/8.

An easy fix would be to add something like:

.admin-bar .dops-modal-wrapper {
  top: 46px;

  @include breakpoint( '>782px' ) {
    top: 32px;
  }
}

@enejb
Copy link
Member Author

enejb commented Mar 9, 2020

Can you help me replicate the Black bar? What browser were you using?
Maybe the better solution could be to just make sure that the modal has a higher z-index. So that it shows up on top?

@keoshi
Copy link
Contributor

keoshi commented Mar 9, 2020

@enejb I was logged into the site, so the WP admin bar was showing. I'd rather not have something overlap with the core admin bar, but let me know if you think that's a better option.

_inc/client/at-a-glance/style.scss Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! Admin Page React-powered dashboard under the Jetpack menu and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 11, 2020
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

You still have a few spaces left. I would recommend setting up your IDE so it transforms those on save, that should save you some trouble!

@enejb enejb requested a review from keoshi March 12, 2020 12:37
@enejb
Copy link
Member Author

enejb commented Mar 12, 2020

I fixed the adminbar overlap and the spacing issue.

@enejb enejb requested a review from jeherve March 12, 2020 12:38
@enejb enejb added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Mar 12, 2020
@github-actions
Copy link
Contributor

Howdy! The Jetpack team has disappeared for a few days to a secret island lair to concoct new ways to make Jetpack one hundred billion percent better. As a result, your Pull Request may not be reviewed right away. Do not worry, we will be back next week to look at your work! Thank you for your understanding.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I removed more spaces in b150c3f

This should be good to go now.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 12, 2020
@jeherve jeherve added [Status] Needs Design Review Design has been added. Needs a review! and removed [Status] Needs Design labels Mar 12, 2020
Copy link
Contributor

@keoshi keoshi left a comment

Choose a reason for hiding this comment

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

LGTM

@jeherve jeherve removed the [Status] Needs Design Review Design has been added. Needs a review! label Mar 12, 2020
@jeherve jeherve merged commit e71b4a2 into master Mar 12, 2020
@jeherve jeherve deleted the fix/disconnect-mobile-8726 branch March 12, 2020 16:34
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 12, 2020
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Mar 20, 2020
jeherve added a commit that referenced this pull request Mar 20, 2020
jeherve added a commit that referenced this pull request Mar 31, 2020
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu Connect Flow Connection banners, buttons, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin Page: cannot disconnect Jetpack from WordPress.com on mobile device
5 participants