Skip to content

Always display the mobile nav menu when disconnecting sites#14925

Merged
jeherve merged 5 commits intomasterfrom
fix/disconnect-mobile-8726
Mar 12, 2020
Merged

Always display the mobile nav menu when disconnecting sites#14925
jeherve merged 5 commits intomasterfrom
fix/disconnect-mobile-8726

Conversation

@enejb
Copy link
Copy Markdown
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.

@enejb
Always display the mobile nav menu when disconnecting sites
135e722 
Make the cancel and disconnect buttons always visible on mobile devices.
@enejb enejb added [Status] Needs Review This PR is ready for review. [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
Copy Markdown
Collaborator

jetpackbot commented Mar 9, 2020 •
edited
Loading

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

keoshi
keoshi requested changes Mar 9, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

enejb commented Mar 9, 2020 •
edited
Loading

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
Copy Markdown
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.

jeherve
jeherve requested changes Mar 11, 2020
View reviewed changes
@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. Admin Page React-powered dashboard under the Jetpack menu and removed [Status] Needs Review This PR is ready for review. labels Mar 11, 2020
jeherve
jeherve requested changes Mar 12, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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 This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 12, 2020
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 12, 2020

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.

jeherve
jeherve approved these changes Mar 12, 2020
View reviewed changes
Copy link
Copy Markdown
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 This PR is ready for review. 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
keoshi
keoshi approved these changes Mar 12, 2020
View reviewed changes
Copy link
Copy Markdown
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
Changelog: add #14925
122a578
jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* 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

Reviewers

@keoshi keoshi keoshi approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@enejb enejb

Labels

Admin Page React-powered dashboard under the Jetpack menu Connect Flow Connection banners, buttons, ...

Projects

None yet

Milestone

8.4

Development

Successfully merging this pull request may close these issues.

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

5 participants

@enejb @jetpackbot @keoshi @jeherve @matticbot