Skip to content

Repeat Visitor: register new block and assets#11224

Merged
jeherve merged 9 commits intomasterfrom
try/visited-block
Feb 18, 2019
Merged

Repeat Visitor: register new block and assets#11224
jeherve merged 9 commits intomasterfrom
try/visited-block

Conversation

@jsnmoon
Copy link
Copy Markdown
Contributor

@jsnmoon jsnmoon commented Jan 29, 2019 •
edited by bluefuton
Loading

Changes proposed in this Pull Request:

This change adds a new "Repeat Visitor" block that controls its Inner Block visibility based on how often a visitor has viewed the page. It can be configured to show its content either above or below a certain view count threshold.

The corresponding wp-calypso PR is here.

Testing instructions:

  1. Spin up a Jurassic Ninja instance.

  2. Connect Jetpack to your WPCOM account.

  3. Navigate to Settings->Jetpack Constants, check JETPACK_BETA_BLOCKS, and Save.

  4. Add the Visited block to a post or page. Nest whatever block within the Visited block.

  5. Save a post and open it.

  6. Check the cookie value for jp-visit-counter; it should have incremented from 0 to 1.

  7. Refresh the page multiple times and ensure that the jp-visit-counter continues to increment. Ensure that your content becomes visible after it crosses the minimum view threshold (and vice versa for maximum view threshold).

Proposed changelog entry for your changes:

  • Adds a Repeat Visitor block that controls block visibility based on how often a visitor has viewed the page.

@jsnmoon jsnmoon self-assigned this Jan 29, 2019
@jsnmoon jsnmoon requested a review from a team January 29, 2019 22:15
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Jan 29, 2019

D23649-code. (newly created revision)

@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Jan 29, 2019 •
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: March 5, 2019.
Scheduled code freeze: February 26, 2019

Generated by 🚫 dangerJS against 2409f8c

@jeherve jeherve added [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Repeat Visitor labels Jan 30, 2019
@jsnmoon jsnmoon force-pushed the try/visited-block branch 6 times, most recently from 96606dc to 7df525c Compare February 1, 2019 22:49
@jsnmoon jsnmoon changed the title Visited: Register block and assets Previously Visited: Register new block and assets Feb 4, 2019
@jsnmoon jsnmoon added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Feb 4, 2019
@jsnmoon jsnmoon requested review from a team February 5, 2019 19:51
@jeherve jeherve added this to the 7.1 milestone Feb 5, 2019
@enejb
Copy link
Copy Markdown
Member

enejb commented Feb 5, 2019

🎉
I tested this and it works really well when we have multiple blocks on a single page.
As well as when we have different blocks on different pages.

I just wonder if we should have something that removes the cookie from the users. Or stops the counter at a reasonable maximum ? 100 hits?

Also do you know if we we need to update our GDRP documentation when we ship this feature?

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Feb 6, 2019

do you know if we we need to update our GDRP documentation when we ship this feature?

Yes, I think we'll need to mention the cookie in the support doc for that block.

jeherve
jeherve requested changes Feb 6, 2019
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'd have a few minor remarks (see below), as well as 2 small things that confused me when trying the block:

  • It seems the block visibility is not affected when you are logged in to your WordPress account; I kept seeing the block even though I had visited the page multiple times. As soon as I logged out, the block disappeared. I understand why because I could see the cookie value, but I think site owners may be confused by this behaviour, and expect:
    1. To see a banner telling them that since they are logged in, they can always see the block.
    2. The counter should only increment when you are logged out. This would allow me to really test the block; I would log out and refresh the page 3 times to make sure my block is working. If we want to increment the counter regardless of whether you are logged in or not, I think we should use the banner I mention in "1" to show the current counter status.
  • It wasn't clear to me that the counter was meant as a "more or equal" or "less or equal". Maybe we should update the wording to make that clearer?

@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 Feb 6, 2019
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Feb 6, 2019

jsnmoon, Your synced wpcom patch D23649-code has been updated.

@jsnmoon
Copy link
Copy Markdown
Contributor Author

jsnmoon commented Feb 6, 2019

I just wonder if we should have something that removes the cookie from the users. Or stops the counter at a reasonable maximum? 100 hits?

Currently, the cookie value is set to expire after 6 months as seen here. Do you think we should consider a shorter max cookie age?

(I'm a little lukewarm on the idea of setting a max limit for the visit counter, given that I don't really have a good gauge for what a reasonable maximum might be.)

Also do you know if we need to update our GDRP documentation when we ship this feature? (@enejb)

Yes, I think we'll need to mention the cookie in the support doc for that block. (@jeherve)

Absolutely agreed. I went ahead and posted internally for privacy guidance here: p4H3ND-NR.

@jsnmoon
Copy link
Copy Markdown
Contributor Author

jsnmoon commented Feb 6, 2019 •
edited
Loading

It seems the block visibility is not affected when you are logged in to your WordPress account; I kept seeing the block even though I had visited the page multiple times. As soon as I logged out, the block disappeared.

[...] The counter should only increment when you are logged out. This would allow me to really test the block; I would log out and refresh the page 3 times to make sure my block is working. If we want to increment the counter regardless of whether you are logged in or not, I think we should use the banner I mention in "1" to show the current counter status. (@jeherve)

This behavior is a bit bizarre, given that we don't have specific handling for logged-in users. As currently implemented, block visibility should be unaffected by the authentication state of the viewer.

At any rate, I can see how always showing the block to the author could be helpful. Do we have any Gutenblocks that provide this kind of differentiated experience for authors viewing their own post(s)?

It wasn't clear to me that the counter was meant as a "more or equal" or "less or equal". Maybe we should update the wording to make that clearer?

The phrasing for the counter was actually from the editorial team... Nonetheless, I can see how it might confuse our users.

@jsnmoon jsnmoon added [Status] Needs Review This PR is ready for review. [Status] Needs Design Review Design has been added. Needs a review! labels Feb 6, 2019
jeherve
jeherve requested changes Feb 15, 2019
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.

Quick note about the cookie's name, as we discussed earlier.

@jeherve jeherve 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 Feb 15, 2019
@jeherve jeherve modified the milestones: 7.1, 7.2 Feb 18, 2019
@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 Feb 18, 2019
jeherve
jeherve approved these changes Feb 18, 2019
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.

This is looking good and tests well now. Merging here and on WordPress.com; that should make testing easier.

@jeherve jeherve merged commit 139a47d into master Feb 18, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 18, 2019
@jeherve jeherve deleted the try/visited-block branch February 18, 2019 12:25
@bluefuton
Copy link
Copy Markdown
Contributor

bluefuton commented Feb 18, 2019

Thank you for your help getting this shipped @jeherve! 🚢

@bluefuton bluefuton removed the [Status] Needs Design Review Design has been added. Needs a review! label Feb 18, 2019
kraftbj added a commit that referenced this pull request Mar 25, 2019
@kraftbj
Add CL for #11224
c96c504
kraftbj added a commit that referenced this pull request Mar 25, 2019
@kraftbj
Add testing instructions for #11224
bd95d57
kraftbj added a commit that referenced this pull request Mar 27, 2019
@kraftbj
Add initial 7.2 changelog to master (#11690)
2227bfd 
* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Add CL for #11224

* Add CL for #11426

* Add CL for #11442

* Add testing instructions for #11224

* Add CL for #11451

* Reclassify CL item

* Add testing instructions for #11451

* Add CL for #11486

* Add CL for #11418

* Add CL for #11524

* Add CL and testing instructions for #11449

* Add CL for #11460

* Add CL for #11520 and #11582

* Add CL for #11531

* Add CL #11644

* Add testing instructions for #11644

* Add testing instructions for #11644

* Add CL for #11618

* Uniform changelog lines

* CL #11679

* CL #11661

* CL #11654

* CL #11645

* CL #11643

* CL #11636

* CL #11635 and for other PHPCS commits

* CL #11627

* CL #11626

* CL #11598

* CL #11596

* Remove nested items for shortcopy. I don't believe the detailed list is helpful

* CL #11570

* CL #11569

* CL #11560

* CL #11558

* CL #11555

* CL #6704

* CL #11298

* CL #11324

* CL #11443

* CL #11484

* CL #11516

* CL #11529

* Expand Ads block enhancement CL item
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@simison simison simison left review comments

@jeherve jeherve jeherve approved these changes

Assignees

@jsnmoon jsnmoon

Labels

[Block] Repeat Visitor [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files

Projects

None yet

Milestone

7.2

Development

Successfully merging this pull request may close these issues.

9 participants

@jsnmoon @matticbot @jetpackbot @enejb @jeherve @scruffian @simison @bluefuton @kraftbj