Feature/finalize admin view for dashboard#1263
Conversation
…dmin-view-for-dashboard # Conflicts: # lib/i18n/en.i18n.json
|
@apinf/developers, please review. |
| {{ > requestsOverTimeChart }} | ||
| </div> | ||
| <div> | ||
| <div class="col-lg-12"> |
There was a problem hiding this comment.
Is this Bootstrap class necessary when there are no other column sizes declared? E.g. does Bootstrap default to 12 columns?
There was a problem hiding this comment.
Yes, it is. Bootstrap does default to 12 columns if we specify one of lg, md, sm, xs columns classes.
| @@ -1,6 +1,16 @@ | |||
| <template name="dataTable"> | |||
There was a problem hiding this comment.
While we are here, lets give this template a more specific name. E.g.
dashboardDataTable
dashboard/client/dashboard.html
Outdated
| <div class="row"> | ||
| <div class="col-lg-12 col-md-12 col-sm-12"> | ||
| <h1 class="dashboard-title"> | ||
| {{_ "dashboard_MainTitle" }} |
There was a problem hiding this comment.
First letter of camelCase should be lowercase:
"dashboard_mainTitle"
instead of
"dashboard_MainTitle"
|
The main feedback on this PR is that we need to be consistent with our i18n keys. The keys are not very consistent right now, so we should pick a convention and stick to it. My suggestion is to go with camelCase, where the first letter is lowercase. |
|
The only significant change I have recommended is to move markup and logic from the charts.html/.js/.less into dashboard.html/js/less. This is partially because the use of 'layout' is confusing, and also the extra charts.* files are somewhat unnecessary. See PR #1282 for discussion about this change, which can be made there instead. Otherwise, this PR looks ready to merge. |
|
I have done this change in #1282
|
|
Thanks @frenchbread 🎁 |
|
@brylie, @frenchbread has now those structure changes in his PR. i18n keys updated. |
|
Merged. It looks like our i18n keys are about 50% camel case, 50% camel case with a capitalized first letter. We should consider whether/how to make these consistent. |
3 participants
Closes #1260
Closes #674
Proposed changes: