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

JITMs: Fix styling to work better with a popular plugin #10539

Merged
merged 1 commit into from
Nov 5, 2018

Conversation

withinboredom
Copy link
Contributor

@jeffgolenski and I worked together to fix some styling issues.

Fixes #9221

Changes proposed in this Pull Request:

Fix some spacing when a popular plugin is present:

screen shot 2018-11-05 at 5 50 24 pm

Testing instructions:

  1. Turn on Hello Dolly on a site with no plan and akismet inactive
  2. View the comments page
  3. See a jitm that doesn't collide with the hello dolly text

Proposed changelog entry for your changes:

No changelog entry is needed except maybe: Improved plugin compatibility

@withinboredom withinboredom requested a review from a team as a code owner November 5, 2018 16:59
@jetpackbot
Copy link

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS

@withinboredom withinboredom self-assigned this Nov 5, 2018
@withinboredom withinboredom added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Design Review Design has been added. Needs a review! [Pri] Normal [Feature] JITM Just In Time Messages - pop-up tips and suggestions that appear on the dashboard and sidebar. labels Nov 5, 2018
@jeherve jeherve added this to the 6.8 milestone Nov 5, 2018
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.

It worked well for me in Safari, Firefox, Chrome, and on Chrome for Android. 👍

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.

Works great in my testing!

@keoshi keoshi removed the [Status] Needs Design Review Design has been added. Needs a review! label Nov 5, 2018
@jeffgolenski jeffgolenski self-requested a review November 5, 2018 17:51
Copy link
Member

@jeffgolenski jeffgolenski left a comment

Choose a reason for hiding this comment

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

Confirmed: This fix works well on all sized screens — mobile through desktop

screen shot 2018-11-05 at 1 14 01 pm

screen shot 2018-11-05 at 1 14 42 pm

screen shot 2018-11-05 at 1 14 57 pm

@jeherve jeherve modified the milestones: 6.8, 6.7 Nov 5, 2018
@jeherve jeherve merged commit af7011b into master Nov 5, 2018
@jeherve jeherve deleted the fix/hello-dolly branch November 5, 2018 21:26
jeherve pushed a commit that referenced this pull request Nov 5, 2018
@jeffgolenski and I worked together to fix some styling issues.

<!--- Provide a general summary of your changes in the Title above -->

Fixes #9221

#### Changes proposed in this Pull Request:
<!--- Explain what functional changes your PR includes -->

Fix some spacing when a popular plugin is present:

<img width="847" alt="screen shot 2018-11-05 at 5 50 24 pm" src="https://user-images.githubusercontent.com/1883296/48013289-41929d80-e124-11e8-8dea-f6b7de9af1b1.png">

#### Testing instructions:
<!-- Please include detailed testing steps, explaining how to test your change. -->
<!-- Bear in mind that context you working on is not obvious for everyone.  -->
<!-- Adding "simple" configuration steps will help reviewers to get to your PR as quickly as possible. -->
<!-- "Before / After" screenshots can also be very helpful when the change is visual. -->

1. Turn on Hello Dolly on a site with no plan and akismet inactive
2. View the comments page
3. See a jitm that doesn't collide with the hello dolly text

#### Proposed changelog entry for your changes:
<!-- Please do not leave this empty. If no changelog entry needed, state as such. -->

No changelog entry is needed except maybe: Improved plugin compatibility
@jeherve
Copy link
Member

jeherve commented Nov 5, 2018

Cherry-picked to branch-6.7 in 1e807b2

@jeherve
Copy link
Member

jeherve commented Nov 6, 2018

@jeffgolenski @withinboredom I'm seeing some issues now when Hello Dolly isn't installed:

  • On the Dashboard > Updates page:

screenshot 2018-11-06 at 9 55 26

  • On the Appearance > Themes page:

screenshot 2018-11-06 at 9 55 55

It looks like we have some missing padding on the right.

jeherve added a commit that referenced this pull request Nov 6, 2018
…)"

This reverts commit 1e807b2.
Let's not ship this just yet, as we need to fix this issue first:
#10539 (comment)
@jeherve
Copy link
Member

jeherve commented Nov 6, 2018

Until the above is fixed, let's not ship that Hello Dolly fix: I reverted here:
b19f7fb

@jeherve jeherve mentioned this pull request Nov 12, 2018
@jeffgolenski
Copy link
Member

I'll take another look at it soon, @jeherve - thanks for the double check

jeherve added a commit that referenced this pull request Nov 15, 2018
@jeherve jeherve restored the fix/hello-dolly branch December 4, 2018 20:17
@jeffgolenski
Copy link
Member

I just moved this into the early value project to ensure it gets fixed this week!

@jeffgolenski jeffgolenski restored the fix/hello-dolly branch December 8, 2018 20:39
@jeffgolenski
Copy link
Member

@jeherve were you only testing this on Jurassic ninja sites?

I noticed that everything works and looks as intended, except for JN sites, because JN throws some CSS right in-between the screen meta box and the JITM, which is just messing with the JITM styles.

see how it works fine in a sandbox: https://cloudup.com/cMVM6scsT9h

But this CSS messed up the JITM declarations that we have written
screen shot 2018-12-08 at 4 39 28 pm

because it gets in the way of
#screen-meta-links+.jitm-card { margin: rem( 40px ) 1.5385em 0 auto; }

@jeffgolenski
Copy link
Member

@jeherve Here's an example of that I mean: https://cloudup.com/i5wSzcK89pb

@jeherve
Copy link
Member

jeherve commented Dec 10, 2018

I don't recall, sorry. The screenshot above is from a JN site for sure, but I don't remember if I tested somewhere else as well. I'll test your patch in #10840!

@kraftbj kraftbj deleted the fix/hello-dolly branch May 24, 2019 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] JITM Just In Time Messages - pop-up tips and suggestions that appear on the dashboard and sidebar. [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JITMs: be nice to Dolly
6 participants