Skip to content

Add/jitm builder#7058

Merged
gravityrail merged 29 commits intomasterfrom
add/jitm-builder
May 29, 2017
Merged

Add/jitm builder#7058
gravityrail merged 29 commits intomasterfrom
add/jitm-builder

Conversation

@withinboredom
Copy link
Copy Markdown
Contributor

@withinboredom withinboredom commented Apr 25, 2017

Justin Time Messages

Requires: D5613-code to be available first shipped.

Uses an API to determine which JITM to show. At first it may seem like a bad idea to have an ajax call fire on every wp-admin page view. However, the call is extremely lightweight if there is not JITM defined for a page, bailing out nearly instantly.

It's designed to cache a call for 5 minutes by default (unless the JITM being shown sets something differently). If a sync occurs (last_heartbeat option, also adds a last_sync option) it will disregard the cache. This is useful in the case where a user clicks a CTA on a JITM or causes some action that would hide the JITM. Such as enabling Akismet. This seems to trigger a heartbeat instantly and we invalidate the cache. I'm open to any and all suggestions about making this more reliable and resilient.

Testing instructions:

  • You'll need D5613-code, see that testing plan as well.
  • Make sure you do a yarn build
  • Verify defined JITM's work as defined (matches master behavior)
  • Ensure JITM's dismiss properly by product_class (if one VaultPress is dismissed, all of them do)
  • Ensure activating Akismet will prevent the JITM from showing on the "Comments" screen.

Existing JITMs:

  • Akismet: comments page with akismet deactivated
  • VaultPress: update-core page and just after publishing a post with VaultPress deactivated
  • WooCommerce: Woo installed and activated set to USA or Canada, on woo settings page (among others) ... woo services plugin must be deactivated or not installed.

Proposed changelog entry for your changes:

N/A

@withinboredom withinboredom self-assigned this Apr 25, 2017
@withinboredom withinboredom force-pushed the add/jitm-builder branch 4 times, most recently from 2bc37b5 to 5e557fe Compare May 2, 2017 15:21
@withinboredom withinboredom force-pushed the add/jitm-builder branch 5 times, most recently from 671b51b to 79c2937 Compare May 8, 2017 16:48
@withinboredom withinboredom force-pushed the add/jitm-builder branch 4 times, most recently from 29c38ea to d1b6ade Compare May 16, 2017 14:46
@ebinnion
Copy link
Copy Markdown
Contributor

I'm still trying to wrap my head around all of the changes. Lots of red and green.

For some earlier feedback, I have a question. Are we sure that we don't call to get JITMs from the frontend? We'd only want to do that when the wp-admin is loaded, right?

@withinboredom
Copy link
Copy Markdown
Contributor Author

Yeah, I'll double check just to be sure.

@withinboredom
Copy link
Copy Markdown
Contributor Author

Confirmed it will not show on the frontend, should only perform the api call for users on wp-admin.

@ebinnion
Copy link
Copy Markdown
Contributor

I am testing by applying D5613-code to my sandbox as well as this PR to my Jetpack site. Here are a couple of things I notice:

The notice formatting on the Akismet page looks off:

screen shot 2017-05-25 at 3 41 09 pm

The VaultPress continues to show after each post I publish. This may be happening because each post has a unique ID, but that seems a bit buggy to me.

vp_notice_continues_to_show

Perhaps this should be in the WPCOM patch, but for the Akismet nudge, it takes me to a Jetpack.com page that tries to sell me a plan, but I already have a personal plan for this site.

screen shot 2017-05-25 at 3 50 45 pm

I would expect the primary button to either take me to wordpress.com/plugins/setup/$site, or wordpress.com/plugins/akismet/ericbinnion in this case.

Note: This is the same for VaultPress. I get taken to a Jetpack.com page that expects me to buy a personal plan, but I already have a personal plan.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jeffgolenski Just want to double check that we're good to change this since I assume it's used in multiple places.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hmm, yeah, if it's just for jitms, it can be moved easily.

@ashleighaxios
Copy link
Copy Markdown
Contributor

Width fix looks LGTM.

Some other quick things noticed in the above.

There are some subtle alignment, width issues: close icons should align (highlighted in green) & the green border on the separate messages should be the same width (highlighted in yellow). I would be tempted to move these into a new PR once the above is merged since there's a lot in this PR, but defer to you all.
eec8a046-4204-11e7-99c6-76a21acdb1ad_alignment
eecb610a-4204-11e7-9aa9-c957b383be01_alignment

It's also worth noting that in the above example, the language of the JITM isn't translating. Perhaps this is expected with new language for a short period of time?
eecb610a-4204-11e7-9aa9-c957b383be01_language

@withinboredom
Copy link
Copy Markdown
Contributor Author

JITM translation fix is in another PR: #5535

@ashleighaxios
Copy link
Copy Markdown
Contributor

I propose we move this forward then.

@eliorivero
Copy link
Copy Markdown
Contributor

eliorivero commented May 29, 2017

captura de pantalla 2017-05-29 a las 13 24 59

The jitm looks fantastic! Although I'm not a fan of how it's placed next to the tabs, looks crowded. Maybe it would be better if it was on a line of its own.
I'd add

.jetpack-jitm-message {
    display: inline-block;
    width: 100%;
}

so it looks like
captura de pantalla 2017-05-29 a las 13 28 50

And also, the centered text in the button looks awkward and it's harder to consume than in a single line. Adding width: 70% to .jitm-banner__info would make it look like this:
captura de pantalla 2017-05-29 a las 13 30 52

Just some thoughts. Maybe there was another design review to this and was decided to left them like this?

@eliorivero
Copy link
Copy Markdown
Contributor

One more, the ⨉ is misaligned with respect to the tab down arrows. With this

.jitm-banner__action + .jitm-banner__dismiss {
    margin-right: -8px;
    margin-left: 5px;
}

It looks aligned:

captura de pantalla 2017-05-29 a las 13 35 59

captura de pantalla 2017-05-29 a las 13 36 56

@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. and removed [Status] Needs Review This PR is ready for review. labels May 29, 2017
@eliorivero eliorivero added [Status] Ready to Merge Go ahead, you can push that green button! 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 May 29, 2017
Copy link
Copy Markdown
Contributor

@eliorivero eliorivero left a comment

Choose a reason for hiding this comment

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

Works fine!
Rob told me that the styling will be tweaked in a separate PR so this is good to go! 🐑

@gravityrail gravityrail merged commit 5fb4e36 into master May 29, 2017
@gravityrail gravityrail deleted the add/jitm-builder branch May 29, 2017 16:51
@gravityrail gravityrail removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 29, 2017
@jeherve jeherve added [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels May 29, 2017
jeherve added a commit that referenced this pull request May 29, 2017
@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 May 29, 2017
eliorivero pushed a commit that referenced this pull request May 30, 2017
* Changelog: first pass at a changelog for 5.0

* Changelog: delete 4.9 testing list.

* Changelog: update minimum WP version to match ver. in jetpack.php

Fixes #7158

* Changelog: add #6051

* Changelog: add #6753

* Changelog: add #6928

* Changelog: add #6964

* Changelog: add #7014

* Changelog: add #7057

* Changelog: add #7060

* Changelog: add #7068

* Changelog: add #7070

* Changelog: add #7072

* Changelog: add #7071

* Changelog: add release date and post shortlink.

* Changelog: add #7094

* Changelog: add #7100

* Changelog: add #7108

* Changelog: add #7113

* Changelog: add #7123

* Changelog: add #7135

* Changelog: add #7143

* Changelog: add #7151

* Changelog: add #6996

* Changelog: add #7105

* Changelog: add #7132

* Changelog: add #7166

* Changelog: fix typo in 4.9 changelog.

* Changelog: remove older releases' changelogs.

@see p1HpG7-42e-p2

* Changelog: add #7090

* Changelog: add #7095

* Changelog: add #7112

* Changelog: add #7115

* Changelog: add #7122

* Changelog: add #7137

* Changelog: add #7138

* Changelog: add #7140

* Changelog: add #7154

* Changelog: add ##7155

* Changelog: add #7163

* Changelog: add #7167

* Changelog: add #7171

* Changelog: add #7180

* Changelog: add #7181

* Changelog: add #7183

* Changelog: add #7184

* Changelog: add #7189

* Changelog: add #7191

* Changelog: add #7193

* Changelog: add #7198

* Changelog: add #7200

* Changelog: add #7209

* Changelog: add #7212

* Testing list: add instructions for #7115

* Changelog: add #7188

* Changelog: add #7205

* Changelog: add #7225

* Changelog: add #6872

* Changelog: add #7107

* Changelog: add #7118

* Changelog: add #7142

* Changelog: add #7170

* Changelog: add #7210

* Changelog: add #7218

* Changelog: add #7232

* Changelog: add #7211

* Changelog: add #7213

* Changelog: add #7229

* Changelog: add #7230

* Changelog: add #7214

* Draft changelog for 5.0

* Changelog updates: 2nd pass at a clearer changelog.

- Fix typos.
- Use consistent tense and tone across all changelog.
- Remove unclear items.

* Changelog: add #7026

* Changelog: add #7058

* Changelog: add #7125

* Changelog: add #7249

* Changelog: add #7185

* add mentions of image widget migration

* Changelog: add info about new output for CLI command.

* Changelog: add WP version number matching the new Image Widget.
@jeffgolenski jeffgolenski mentioned this pull request May 30, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants