Nav Unification: Display the stats sparkline on WP Admin for Atomic sites #21655

mmtr · 2021-11-05T14:48:14Z

Changes proposed in this Pull Request:

Displays the stats sparkline in the WP Admin sidebar of Atomic sites, effectively achieving both Calypso/WP Admin and Simple/Atomic parity.

Note that the Stats module has been updated to allow a new masterbar param in chart URLs, which produces a slightly different version of the chart suitable for the sidebar (see r124501-wpcom).

Jetpack product discussion

N/A.

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Install Jetpack Beta on an Atomic site and activate to the branch of this PR.
Go to any WP Admin screen of your Atomic site.
Make sure the sparkline chart is displayed right next to the Stats menu.
Make sure the chart matches the one displayed on Calypso screens.
Turn off the Stats module (you can do this on the Blog RC).
Reload the WP Admin screen.
Make sure the sparkline chart doesn't show up.

…ites

test-case-reminder

Here are some suggested test cases for this PR.

Stats

Visit home page, make sure that page view is recorded in stats
Visit home page in AMP view
Visit post page in non-AMP and AMP view

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration file.

matticbot · 2021-11-05T14:48:21Z

Caution: This PR has changes that must be merged to WordPress.com
Hello mmtr! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D69685-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

github-actions · 2021-11-05T14:49:31Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: December 7, 2021.
Scheduled code freeze: November 30, 2021.

projects/plugins/jetpack/modules/masterbar/admin-menu/class-atomic-admin-menu.php

BogdanUngureanu · 2021-11-05T16:29:13Z

Nice work, Miguel! I've followed the testing instructions and looks like there's a small UI issue in Chrome:

Maybe I'm missing something? 🤔

mmtr · 2021-11-05T16:44:30Z

@BogdanUngureanu I cannot replicate that issue. That happens when the masterbar param is missing from the image URL. Have you modified the code by any chance?

This is what I get:

mmtr · 2021-11-05T16:49:51Z

Scratch that, just found out that the admin-bar-hours-scale chart (1x) doesn't support the masterbar param, so I guess you were getting the 1x image while I was getting the 2x image.

I'll remove the fallback to the 1x image, and just use the 2x image, as done for Simple sites:

jetpack/projects/plugins/jetpack/modules/masterbar/admin-menu/class-wpcom-admin-menu.php

Line 241 in 90d6743

    
           esc_url( site_url( 'wp-includes/charts/admin-bar-hours-scale-2x.php?masterbar=1&s=' . get_current_blog_id() ) ),

Update: done in 669afa4.

BogdanUngureanu · 2021-11-08T10:16:14Z

Tests great for me now in WP-Admin, great job Miguel! One question: do you happen to know why we don't display the sparkline in Calypso?

jeherve

Do we need to take into account sites where the Stats module will be inactive?

projects/plugins/jetpack/modules/masterbar/admin-menu/class-atomic-admin-menu.php

mmtr · 2021-11-08T16:11:58Z

do you happen to know why we don't display the sparkline in Calypso?

I'd swear we were displaying it (you can see some screenshots in Automattic/wp-calypso#56542), so I wonder if that's due to a recent change. I'll investigate.

…plication

mmtr · 2021-11-08T16:53:00Z

Do we need to take into account sites where the Stats module will be inactive?

Yup! I totally missed that. Handle it in 8efff91

mmtr · 2021-11-09T08:43:00Z

do you happen to know why we don't display the sparkline in Calypso?

I'd swear we were displaying it (you can see some screenshots in Automattic/wp-calypso#56542), so I wonder if that's due to a recent change. I'll investigate.

That was it, should've been fixed in Automattic/wp-calypso#57814.

BogdanUngureanu · 2021-11-09T10:08:36Z

Thanks for the update! I had another look and the Sparkline now appears both in Calypso and WP-Admin! However, I did notice that in Calypso the vertical bar is higher and whiter than in WP-Admin:

LE: Looks like the line is also positioned a bit differently.

projects/plugins/jetpack/modules/stats.php

mmtr · 2021-11-09T10:30:59Z

However, I did notice that in Calypso the vertical bar is higher and whiter than in WP-Admin

I think that's unrelated to this PR, since it's happening on Simple sites as well. I created a separate issue: Automattic/wp-calypso#57820

samiff

Tested well for me. Also did a quick check on a non-WPcom Atomic site and didn't notice any issues there 👍

…ats_get_image_chart_src` exists

jeherve · 2021-11-11T12:09:34Z

~~Could you try to update this branch with latest master to include the changes from #21697? It should hopefully solve the issue with the Gardening action on this PR.~~
Scratch that, I think that's something we'll need to fix in the monorepo. See #21712.

Copons · 2021-11-11T13:08:12Z

r234861-wpcom

* master: (22 commits) VideoPress: reload block on rating change (#21653) Assets: Changelog for package version 1.12.0 (#21744) assets: Add `wp_register_script` wrapper (and then use it everywhere) (#21689) eslint-config-target-es: Configure mirror repo (#21731) Use monorepo `validate-es` script to validate Webpack builds (#21729) Backup: Replace daily backup references/upsell links with new real-time products (#21715) Likes: reimplement non-admin portions without jQuery (#21726) Autoloader: Not activated autoload queue is false (#21517) Sync: add a new method, do_only_first_initial_sync (#21676) webpack-config: Configure minifier to preserve translator comments (#21667) webpack-config: Use `@automattic/babel-plugin-preserve-i18n` (#21700) Create eslint-config-target-es JS package (#21660) webpack-config: Fork calypso-build's mini-css-with-rtl plugin (#21595) Allow /sites/${site}/external-media/copy/pexels to insert post meta data (#21659) jetpack: Don't set Webpack's `output.pathinfo` in production builds (#21727) Boost: Implement support for loading stylesheets when JavaScript is disabled in the context Critical CSS being enabled (#21713) RNA: export the Connection store (#21388) Display notice when user has unactivated product license keys (#21474) Gardening: ensure it can use Composer (#21712) Nav Unification: Display the stats sparkline on WP Admin for Atomic sites (#21655) ...

Nav Unification: Display the stats sparkline on WP Admin for Atomic s…

c26a1cc

…ites

mmtr self-assigned this Nov 5, 2021

matticbot added the Touches WP.com Files label Nov 5, 2021

test-case-reminder bot reviewed Nov 5, 2021

View reviewed changes

github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Nov 5, 2021

BogdanUngureanu reviewed Nov 5, 2021

View reviewed changes

projects/plugins/jetpack/modules/masterbar/admin-menu/class-atomic-admin-menu.php Outdated Show resolved Hide resolved

Use 2x image only

669afa4

jeherve reviewed Nov 8, 2021

View reviewed changes

projects/plugins/jetpack/modules/masterbar/admin-menu/class-atomic-admin-menu.php Show resolved Hide resolved

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Needs Team Review labels Nov 8, 2021

Check if Stats module is enabled, and refactor logic to avoid code du…

8efff91

…plication

mmtr added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Needs Team Review 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 Nov 8, 2021

BogdanUngureanu reviewed Nov 9, 2021

View reviewed changes

projects/plugins/jetpack/modules/stats.php Show resolved Hide resolved

BogdanUngureanu reviewed Nov 9, 2021

View reviewed changes

projects/plugins/jetpack/modules/stats.php Show resolved Hide resolved

mmtr mentioned this pull request Nov 9, 2021

Stats: Sparkline is different between Calypso and WP Admin Automattic/wp-calypso#57820

Open

BogdanUngureanu previously approved these changes Nov 9, 2021

View reviewed changes

mmtr removed the [Status] Needs Team Review label Nov 9, 2021

mmtr added this to the jetpack/10.4 milestone Nov 9, 2021

Merge branch 'master' into add/nav-unification-stats-sparkline-atomic

eadfa7f

samiff self-requested a review November 9, 2021 19:11

samiff previously approved these changes Nov 9, 2021

View reviewed changes

samiff 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 Nov 9, 2021

Only display the sparkline in the sidebar for Atomic sites if the `st…

70913e0

…ats_get_image_chart_src` exists

Copons dismissed stale reviews from samiff and BogdanUngureanu via 70913e0 November 11, 2021 11:04

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 11, 2021

Merge branch 'master' into add/nav-unification-stats-sparkline-atomic

050aff2

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! 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 Nov 11, 2021

jeherve approved these changes Nov 11, 2021

View reviewed changes

BogdanUngureanu merged commit bbc0207 into master Nov 11, 2021

BogdanUngureanu deleted the add/nav-unification-stats-sparkline-atomic branch November 11, 2021 12:57

anomiex removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nav Unification: Display the stats sparkline on WP Admin for Atomic sites #21655

Nav Unification: Display the stats sparkline on WP Admin for Atomic sites #21655

mmtr commented Nov 5, 2021 •

edited

Loading

test-case-reminder bot left a comment •

edited by mmtr

Loading

matticbot commented Nov 5, 2021

github-actions bot commented Nov 5, 2021 •

edited

Loading

BogdanUngureanu commented Nov 5, 2021

mmtr commented Nov 5, 2021

mmtr commented Nov 5, 2021 •

edited

Loading

BogdanUngureanu commented Nov 8, 2021

jeherve left a comment

mmtr commented Nov 8, 2021

mmtr commented Nov 8, 2021

mmtr commented Nov 9, 2021 •

edited

Loading

BogdanUngureanu commented Nov 9, 2021 •

edited

Loading

mmtr commented Nov 9, 2021

samiff left a comment

jeherve commented Nov 11, 2021 •

edited

Loading

Copons commented Nov 11, 2021

Nav Unification: Display the stats sparkline on WP Admin for Atomic sites #21655

Nav Unification: Display the stats sparkline on WP Admin for Atomic sites #21655

Conversation

mmtr commented Nov 5, 2021 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

test-case-reminder bot left a comment • edited by mmtr Loading

Choose a reason for hiding this comment

Stats

matticbot commented Nov 5, 2021

github-actions bot commented Nov 5, 2021 • edited Loading

BogdanUngureanu commented Nov 5, 2021

mmtr commented Nov 5, 2021

mmtr commented Nov 5, 2021 • edited Loading

BogdanUngureanu commented Nov 8, 2021

jeherve left a comment

Choose a reason for hiding this comment

mmtr commented Nov 8, 2021

mmtr commented Nov 8, 2021

mmtr commented Nov 9, 2021 • edited Loading

BogdanUngureanu commented Nov 9, 2021 • edited Loading

mmtr commented Nov 9, 2021

samiff left a comment

Choose a reason for hiding this comment

jeherve commented Nov 11, 2021 • edited Loading

Copons commented Nov 11, 2021

mmtr commented Nov 5, 2021 •

edited

Loading

test-case-reminder bot left a comment •

edited by mmtr

Loading

github-actions bot commented Nov 5, 2021 •

edited

Loading

mmtr commented Nov 5, 2021 •

edited

Loading

mmtr commented Nov 9, 2021 •

edited

Loading

BogdanUngureanu commented Nov 9, 2021 •

edited

Loading

jeherve commented Nov 11, 2021 •

edited

Loading