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

Add a First View introduction overlay to the Stats section #6717

Merged
merged 46 commits into from
Jul 14, 2016

Conversation

mjuhasz
Copy link
Contributor

@mjuhasz mjuhasz commented Jul 12, 2016

This PR adds a First View introduction overlay to the Stats section.

The First View introduction overlay is shown to users when they first visit the Stats section.

screencapture at tue jul 12 15 24 30 edt 2016

Clicking the "Got it!" button will hide the First View and show the Stats section.

If the "Don't show this again" checkbox is checked, the First View will never been shown to the user again. If it is not checked, the First View will be shown whenever the user visits the Stats section again.

If the "Don't show this again" checkbox was checked, you can delete the calypso_store IndexedDB table in order to reset the Redux state and allow the First View to be shown again.

Testing notes:

  • All browsers (desktop and mobile) should work
  • Either navigating to the stats section via the sidebar or directly accessing a /stats URL will show the first view
  • Clicking "Got it!" should hide the first view
    • If the "Don't show this again" checkbox is checked, the first view will never reappear (even after a reload of Calypso)
    • If the "Don't show this again" checkbox is not checked, the first view will reappear the next time the stats section is viewed
  • Navigating away to a different section and then back to stats without clicking "Got it!" should show the first view again

Support for other sections will be possible once #6651 is completed (Reduxifying <Main>).

Test live: https://calypso.live/?branch=add/stats-first-view

@mjuhasz mjuhasz added [Feature] Stats Everything related to our analytics product at /stats/ [Status] In Progress NUX labels Jul 12, 2016
@shaunandrews
Copy link
Contributor

I pushed some design changes that look like this:

screen shot 2016-07-13 at 4 58 19 pm

I'm still not done with it (I gotta figure out what to do with the illustration), but I think using a card really improves the contrast and legibility of the text.

@mattsherman
Copy link
Contributor

Okay, small update... the "half-loaded" state issue that @shaunandrews found only happens in Safari Private Mode. I believe this is because IndexedDB and localStorage are not available when in private mode.

@shaunandrews
Copy link
Contributor

Here's where I ended up with the design changes:

screen shot 2016-07-14 at 11 21 40 am

@shaunandrews shaunandrews removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Jul 14, 2016
@ehg
Copy link
Member

ehg commented Jul 14, 2016

👍 from me, @mcsf could you have a quick look at the actionLog bits, to make sure we're not going to break anything with #6417 :)


export function switchedFromDifferentSection( state ) {
const section = state.ui.section;
const routeSets = filter( getActionLog( state ), entry => entry.type === 'ROUTE_SET' );
Copy link
Member

Choose a reason for hiding this comment

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

We should be using the proper ROUTE_SET constant from action-types, rather than the string representation.

While we're here: if lodash's filter is only used in its filter( collection, predicate ) form, then it could be replaced with the native implementation: getActionLog( state ).filter( entry => … ). An other usage would be the convenient: filter( getActionLog( state ), { type: ROUTE_SET } ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, fixed!

@mcsf
Copy link
Member

mcsf commented Jul 14, 2016

could you have a quick look at the actionLog bits, to make sure we're not going to break anything

Nothing jumps out. This PR merely consumes getActionLog, and #6417's intervention to action-log/reducer ended up only allowing overrides but not implementing any for ROUTE_SET — meaning we keep collecting the same actions that this PR expects.

let isAbleToAccessLocalStorage = true;

try {
localStorage.checkIfFirstViewPreferencesCanBeCached = null;
Copy link
Member

Choose a reason for hiding this comment

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

localStorage is on the window object, so this would break SSR constraints. Do we actually want to test for localStorage here? We use localForage, I wonder if there's an established way to check elsewhere in Calypso.

Also, putting this in the selector worries me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ehg We will pull this back out of the PR and tackle it in a separate PR where we can discuss more and come to the best solution, taking into account things like SSR. Thanks for pointing this out and questioning it!

@mjuhasz
Copy link
Contributor Author

mjuhasz commented Jul 14, 2016

Marking this as Ready to Merge as per feedback (thumbs-up) from reviewers above. Thank you for your contribution!

@mjuhasz mjuhasz added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 14, 2016
@mjuhasz mjuhasz merged commit df518e9 into master Jul 14, 2016
@mjuhasz mjuhasz deleted the add/stats-first-view branch July 14, 2016 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Stats Everything related to our analytics product at /stats/ NUX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants