-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Media: Update media over storage limit error #4333
Conversation
3b0a0e1
to
d932a45
Compare
return null; | ||
} | ||
return ( | ||
<NoticeAction href="#" onClick={ this.navigateToPlans } onMount={ this.countStorageErrorImpression } > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: Kinda wish we would render a <button />
from <NoticeAction />
if we didn't want to specify a href
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we can improve usage here with the NoticeAction. Forcing an href for non-external links also requires us to prevent the default action.
Seems that analytics related to notice display and action are common enough that, perhaps in a separate pull request, we offer enhanced notice components with this functionality baked in. Thoughts? |
analytics.tracks.recordEvent( 'calypso_upgrade_nudge_cta_click', { | ||
cta_name: 'plan-media-storage-error' | ||
} ); | ||
page( `/plans/${ this.props.siteSlug }` ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the role of my inner @apeatling ( 😄 ), do we worry that by not opening the page in a new tab, there's a feeling of uncertainty by being navigated away from the editor when clicking the call-to-action?
Behavior works as expected. 👍 One thing I noticed in testing - when deleting media from the media library, we don't update the storage progress bar display in the media modal. I might imagine it could be a common flow for users to "purge" unneeded media and expect to see the freed storage reflected in the progress bar. |
d932a45
to
fe30146
Compare
@@ -23,6 +24,12 @@ export default React.createClass( { | |||
}; | |||
}, | |||
|
|||
componentDidMount() { | |||
if ( this.props.onMount ) { | |||
this.props.onMount(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any other way we can handle this? Seems like a potential avenue for problematic usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was thinking of a more generic metrics component wrapper if we do want to track when certain components are rendered. Edit: see #4398
fe30146
to
d7ace8f
Compare
Outstanding future improvements for future PRs
I'll wait until #4398 lands before this is set back to needs review. |
844fd03
to
8010515
Compare
Looks good 👍 |
Thanks for the reviews! I'm going to squash. |
438dd75
to
acc4599
Compare
🎉 |
This PR fixes #3898 and provides a more descriptive error message when you upload a media item and are out of space. Click on the notice action "Upgrade Plan" should take you to the plans page.
Desktop
Mobile: ( Note that our notices don't handle text + action + dismiss buttons nicely )
(And here when it breaks to the next line )
Testing
cc @rralian @mtias @artpi @adambbecker @retrofox @aduth @apeatling