Skip to content

At a glance: add Activity Log card#8199

Merged
eliorivero merged 4 commits intomasterfrom
add/activity-log-dashboard-card
Nov 22, 2017
Merged

At a glance: add Activity Log card#8199
eliorivero merged 4 commits intomasterfrom
add/activity-log-dashboard-card

Conversation

@eliorivero
Copy link
Copy Markdown
Contributor

@eliorivero eliorivero commented Nov 16, 2017 •
edited
Loading

Fixes #8132

Changes proposed in this Pull Request:

  • introduces a new card Activity to point user to Activity Log

Dev Mode

captura de pantalla 2017-11-17 a la s 11 00 28

Free, Personal and Premium plans

captura de pantalla 2017-11-20 a la s 18 44 19

Professional plan

captura de pantalla 2017-11-17 a la s 10 58 13

Testing instructions:

  • test this with a Professional plan and without it

@eliorivero eliorivero added [Feature] Activity Log [Status] In Progress Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Nov 16, 2017
@eliorivero eliorivero self-assigned this Nov 16, 2017
@eliorivero eliorivero requested a review from a team as a code owner November 16, 2017 23:00
@eliorivero eliorivero force-pushed the add/activity-log-dashboard-card branch 2 times, most recently from 9c79494 to b873bb1 Compare November 17, 2017 13:57
@eliorivero eliorivero added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Nov 17, 2017
@eliorivero eliorivero force-pushed the add/activity-log-dashboard-card branch from b873bb1 to 72d0451 Compare November 17, 2017 13:59
@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Nov 17, 2017

Will review but...

We're trying to introduce prop-types in this one #8158

oskosk
oskosk reviewed Nov 20, 2017
View reviewed changes
_inc/client/at-a-glance/index.jsx
</div>
<div className="jp-at-a-glance__right">
<DashScan { ...settingsProps } siteRawUrl={ this.props.siteRawUrl } />
<DashProtect { ...settingsProps } />
Copy link
Copy Markdown
Contributor

@oskosk oskosk Nov 20, 2017

Choose a reason for hiding this comment

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

Looks like this.props.siteRawUrl is empty ("" ) at this point. Probably it wasn't being used and/or passed down from its parent.

Copy link
Copy Markdown
Contributor Author

@eliorivero eliorivero Nov 20, 2017

Choose a reason for hiding this comment

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

That's odd @oskosk I'm seeing this defined at all times. I added a console.log at the beginning of render() and it shows the function perfectly.

Copy link
Copy Markdown
Contributor

@oskosk oskosk Nov 20, 2017

Choose a reason for hiding this comment

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

weird...

@MichaelArestad
Copy link
Copy Markdown
Contributor

MichaelArestad commented Nov 20, 2017

@eliorivero the other cards show an Upgrade notice when they are not available on the current plan. Can you take a look at how those work and try an upgrade notice for folks on the JP free plan?

@eliorivero
Copy link
Copy Markdown
Contributor Author

eliorivero commented Nov 20, 2017

Definitely. I didn't add this because it wasn't in the mockup but I'll add it now.

@MichaelArestad
Copy link
Copy Markdown
Contributor

MichaelArestad commented Nov 20, 2017

@eliorivero yeah that was my fault. I forgot about those until I started switching around plan types.

@eliorivero eliorivero force-pushed the add/activity-log-dashboard-card branch from 72d0451 to ee0af00 Compare November 20, 2017 21:37
@eliorivero
Copy link
Copy Markdown
Contributor Author

eliorivero commented Nov 20, 2017

@MichaelArestad it's now updated 👍 I've updated the corresponding screenshot in this PR too.

@MichaelArestad
Copy link
Copy Markdown
Contributor

MichaelArestad commented Nov 21, 2017

Looks good! 🚢

@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Nov 22, 2017

Tested in another environment. Still keep getting siteRawUrl as an empty string . This time I tested with a fresh installation of WordPress and this PR using the Jetpack Beta Tester Plugin.

image

image

@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Nov 22, 2017

I added a commit fixing the above ^.

@eliorivero
Copy link
Copy Markdown
Contributor Author

eliorivero commented Nov 22, 2017

Thanks @oskosk! updated it to pass only siteRawUrl.

@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Nov 22, 2017

I'm seeing the card like this with Premium and Personal plans:

image

Note the Active label.

@eliorivero
Introduce property isModule in DashItem so cards for elements that ar…
68710c8 
…e not modules, like Activity Log, can opt out of the toggle/button on the right of the header of the cards in dashboard
oskosk
oskosk approved these changes Nov 22, 2017
View reviewed changes
Copy link
Copy Markdown
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.

LGTM!

@oskosk oskosk added the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 22, 2017
@oskosk oskosk removed the [Status] Needs Review This PR is ready for review. label Nov 22, 2017
@oskosk oskosk added this to the 5.6 milestone Nov 22, 2017
@eliorivero eliorivero merged commit baad73d into master Nov 22, 2017
@eliorivero eliorivero deleted the add/activity-log-dashboard-card branch November 22, 2017 22:10
@eliorivero eliorivero removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 22, 2017
jeherve added a commit that referenced this pull request Nov 24, 2017
@jeherve
Changelog: add #8199
778155c
@dereksmart dereksmart mentioned this pull request Nov 28, 2017
Merged
oskosk pushed a commit that referenced this pull request Nov 28, 2017
@jeherve @oskosk
Changelog 5.6: running changelog. (#8192)
7844644 
* Changelog 5.6: create base for changelog.

* Update changelog with 5.5.1 info.

* Changelog: add #7930 and #8238

* Changelog: add #8076

* Changelog: add #8100

* Changelog: add #8117

* Changelog: add #8141

* Changelog: add #8143

* Changelog: add #8147

* Changelog: add #8149

* Changelog: add #8153

* Changelog: add #8173

* Changelog: add #8184

* Changelog: add #8196

* Changelog: add #8199

* Changelog: add #8093

* Changelog: add #8171

* Changelog: add #8182

* Changelog: add #8202, #8222

* Changelog: add #8228

* Changelog: add #8240

* Changelog: add #8251

* remove AL card change
@dereksmart dereksmart mentioned this pull request Dec 3, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaelArestad MichaelArestad Awaiting requested review from MichaelArestad

1 more reviewer

@oskosk oskosk oskosk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@eliorivero eliorivero

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Activity Log

Projects

None yet

Milestone

5.6

Development

Successfully merging this pull request may close these issues.

4 participants

@eliorivero @oskosk @MichaelArestad @jeherve