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.

Sign up for GitHub

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

Jump to bottom

JITMs: display more relevant notices depending on the Jetpack dashboard screen you visit. #10889

Merged
merged 6 commits into from
Dec 21, 2018
Merged

JITMs: display more relevant notices depending on the Jetpack dashboard screen you visit. #10889

merged 6 commits into from
Dec 21, 2018

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Dec 7, 2018

Changes proposed in this Pull Request:

Display more relevant notices depending on the Jetpack dashboard screen you visit.

Testing instructions:

  • You'll need a WordPress.com sandbox.
  • Start by applying D19944-code.
  • Sandbox your Jetpack site with the JETPACK__SANDBOX_DOMAIN constant (reference).
    • If you don't have a WordPress.com sandbox, you can sandbox mine. Through a Jurassic Ninja site with that branch, go to Settings > Jetpack Constants, and add sandboxme.wordpress.com as the sandbox you want to use.
  • Visit Jetpack > Dashboard: a message should appear.
  • Visit Jetpack > Settings: the message should disappear.
  • Visit Jetpack > Plans: a different message should appear.

Proposed changelog entry for your changes:

  • Notices: display more relevant notices depending on the Jetpack dashboard screen you visit.

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Feature] JITM Just In Time Messages - pop-up tips and suggestions that appear on the dashboard and sidebar. labels Dec 7, 2018
@jeherve jeherve added this to the 6.9 milestone Dec 7, 2018
@jeherve jeherve self-assigned this Dec 7, 2018
@jeherve jeherve requested a review from a team as a code owner December 7, 2018 18:45
@jeherve jeherve mentioned this pull request Dec 7, 2018
Closed
4 tasks
@jetpackbot
Copy link

jetpackbot commented Dec 7, 2018 •
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: January 10, 2019.
Scheduled code freeze: January 3, 2019

Generated by 🚫 dangerJS against 80b457e

jeherve added a commit that referenced this pull request Dec 7, 2018
@jeherve
Revert "JITMs: add redux structure and REST API client method to be u…
4ece278 
…sed by the JITM component of the Jetpack Dashboard (Spin-off of #10759). (#10818)"

This reverts commit dabff02.
We will take a different approach, outlined in #10889
@jeherve jeherve mentioned this pull request Dec 7, 2018
Merged
jeherve added a commit that referenced this pull request Dec 7, 2018
@jeherve
JITMs: revert redux changes for new JITM component (#10890)
0a3d37f 
#### Changes proposed in this Pull Request:
Revert "JITMs: add redux structure and REST API client method to be u…sed by the JITM component of the Jetpack Dashboard (Spin-off of #10759). (#10818)"

This reverts commit dabff02.
We will take a different approach, outlined in #10889

#### Testing instructions:

* None

#### Proposed changelog entry for your changes:

* None
@oskosk
Copy link
Contributor

oskosk commented Dec 20, 2018

c99ca033e20d73c730dee78377b064db375989b8 was applied to fix an issue when there was not jitm card before changing routes

jetpack-jitm.js?ver=6.9-alpha:159 Uncaught TypeError: Cannot read property 'remove' of null
    at jetpack-jitm.js?ver=6.9-alpha:159
    at dispatch (jquery.js?ver=1.12.4:3)
    at r.handle (jquery.js?ver=1.12.4:3)

@oskosk
Copy link
Contributor

oskosk commented Dec 20, 2018

When navigating among settings tabs apparently we're querying for JITMs twice

@oskosk
Copy link
Contributor

oskosk commented Dec 20, 2018 •
edited
Loading

hashchange is triggered twice when switching tabs. It's also triggered (and twice) when clicking the current tab. (e.g. you're standing on Sharing, and click Sharing, we fetch JITMs again

image

@AnnaMag AnnaMag force-pushed the update/jitm-refetch-on-hash-change branch 4 times, most recently from ab92b7a to c99ca03 Compare December 20, 2018 21:27
@jeherve jeherve force-pushed the update/jitm-refetch-on-hash-change branch from c99ca03 to c0335a2 Compare December 21, 2018 10:17
@jeherve
Copy link
Member Author

jeherve commented Dec 21, 2018

hashchange is triggered twice when switching tabs.

That seems to be because we first load URLs like jetpack#writing, and then switch to jetpack#/writing.

I pushed a quick fix for that in 3fdcfc0

@jeherve jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Dec 21, 2018
oskosk
oskosk approved these changes Dec 21, 2018
View reviewed changes
Copy link
Contributor

@oskosk oskosk 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 there's a bit of an issue when rapidly switching settings tabs. The previous JITM for the previous JITM will load on the new tab. We'll tackle this on another PR

@oskosk oskosk merged commit a1890e4 into master Dec 21, 2018
@oskosk oskosk deleted the update/jitm-refetch-on-hash-change branch December 21, 2018 14:41
@oskosk oskosk removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Dec 21, 2018
@oskosk
Copy link
Contributor

oskosk commented Dec 21, 2018

The issue mentioned about switching tabs rapidly will be tracked in #11028

@anomiex anomiex mentioned this pull request Dec 19, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oskosk oskosk oskosk approved these changes

Assignees

@jeherve jeherve

Labels
[Feature] JITM Just In Time Messages - pop-up tips and suggestions that appear on the dashboard and sidebar. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
6.9
Development

Successfully merging this pull request may close these issues.
6 participants
@jeherve @jetpackbot @oskosk @matticbot @dereksmart @AnnaMag