-
Notifications
You must be signed in to change notification settings - Fork 365
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
M3-2287: add transfer stats to Linode summary panel #4574
M3-2287: add transfer stats to Linode summary panel #4574
Conversation
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.
Looks great!
return ( | ||
<Notice | ||
text={ | ||
"Couldn't load Linode's network summary. Please, refresh the page to try loading it again." |
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.
In some cases (a Linode without an image, or one that is v. new), this request will fail and reloading the page won't help. imo we should just not show this panel if the request fails, rather than have an error state for it, but if we do want to show it, we should:
- Change the error message to
Network transfer information for this Linode is currently unavailable.
. - Duplicate the logic used in the stats panels elsewhere on LinodeDetail, so that within 24 hours of creation we show a default non-error message.
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.
took care of # 1 , @rulikrulit # 2 is the only feedback remaining
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.
2 is fixed too
<Typography align="center" variant="h2" className={classes.title}> | ||
Monthly Network Transfer | ||
</Typography> | ||
<CircleProgress /> |
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.
Should add the mini
prop here
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.
donzzz
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.
Approved pending Jared feedback for 24h logic
…ulikrulit/manager into feature/M3-2287-transfer-stats
Type of Change