-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: integrate advance analytics charts #1287
feat: integrate advance analytics charts #1287
Conversation
7279e0e
to
ac811d5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1287 +/- ##
==========================================
- Coverage 85.51% 85.48% -0.04%
==========================================
Files 562 564 +2
Lines 12295 12303 +8
Branches 2601 2564 -37
==========================================
+ Hits 10514 10517 +3
- Misses 1722 1728 +6
+ Partials 59 58 -1 ☔ View full report in Codecov by Sentry. |
118b5c3
to
33f1856
Compare
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.
LGTM 👍🏻
}} | ||
loadingMessage={intl.formatMessage({ | ||
id: 'advance.analytics.completions.tab.chart.top.courses.by.completions.loading.message', | ||
defaultMessage: 'Loading top courses by completions chart data', |
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.
Please follow the camelCase pattern to write the IDs of marked strings.
tableColumns: PropTypes.arrayOf(PropTypes.shape({})).isRequired, | ||
tableTitle: PropTypes.string.isRequired, | ||
tableSubtitle: PropTypes.string.isRequired, | ||
enableCSVDownload: PropTypes.bool.isRequired, |
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.
[Suggestion] We could make it a bit more efficient by giving it a default false value. By this, we just need to pass this variable from those charts where we want to enable the download CSV button.
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.
LGTM 👏
edad54e
to
3f786ef
Compare
Description:
JIRA:
For all changes
Only if submitting a visual change