Skip to content
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

Frontend changes for showing tabbed course overview stats #5064

Merged
merged 23 commits into from
Aug 1, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
02da8da
Add namespace-id mapping and make required modifications.
vaidehi44 Jul 21, 2022
8c237a3
Add components to display namespace overview stats in tabs.
vaidehi44 Jul 21, 2022
a0bb519
Modify WikidataOverviewStats container class to adjust css.
vaidehi44 Jul 21, 2022
0c81559
Merge remote-tracking branch 'upstream/master' into namespace-stats-ui
vaidehi44 Jul 21, 2022
23821ba
Some eslint fixes.
vaidehi44 Jul 21, 2022
918faf0
Move stats title function to article utils.
vaidehi44 Jul 23, 2022
8f2d6ca
Refined some code.
vaidehi44 Jul 23, 2022
9cc4b44
Test for overview stats title util function.
vaidehi44 Jul 23, 2022
5670769
Minor changes
vaidehi44 Jul 24, 2022
7dd7f26
Lint fixes
vaidehi44 Jul 24, 2022
f0e4e0b
Fix ruby spec test.
vaidehi44 Jul 24, 2022
c0b47bc
New stats content component.
vaidehi44 Jul 26, 2022
ae7d15d
New UI for tabs.
vaidehi44 Jul 26, 2022
f019e01
Merge branch 'master' of https://github.com/WikiEducationFoundation/W…
vaidehi44 Jul 26, 2022
d8d7bf0
Add method to get namespace id from title.
vaidehi44 Jul 27, 2022
49b696b
Write ruby test for tabbed course stats
vaidehi44 Jul 28, 2022
f7c9833
Change tab id
vaidehi44 Jul 28, 2022
c170b1c
Update format method for all course stats
vaidehi44 Jul 28, 2022
82d042f
Merge remote-tracking branch 'upstream/master' into namespace-stats-ui
vaidehi44 Jul 28, 2022
8aea637
Update renderZero prop for all namespace attributes
vaidehi44 Jul 28, 2022
1639e9f
Update feature test
vaidehi44 Jul 29, 2022
34b8c98
Handle rendering of tabs if there's only one stats object
vaidehi44 Jul 30, 2022
6f58d37
Fix some Lint errors
vaidehi44 Jul 30, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import React from 'react';
import PropTypes from 'prop-types';
import OverviewStat from './overview_stat';

const NamespaceOverviewStats = ({ statistics }) => {
return (
<div className="stat-display">
<OverviewStat
id="articles-created"
className="stat-display__value"
stat={statistics.new_count}
statMsg={I18n.t('metrics.articles_created')}
renderZero={false}
/>
<OverviewStat
id="articles-edited"
className="stat-display__value"
stat = {statistics.edited_count}
statMsg={I18n.t('metrics.articles_edited')}
renderZero={true}
/>
<OverviewStat
id="total-edits"
className="stat-display__value"
stat = {statistics.revision_count}
statMsg={I18n.t('metrics.edit_count_description')}
renderZero={true}
/>
<OverviewStat
id="student-editors"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the user count needs to be in any namespace-specific stats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't it give insight of how many students worked on particular tracked namespace?

className="stat-display__value"
stat={statistics.user_count}
statMsg={I18n.t('courses.student_editors')}
renderZero={true}
/>
<OverviewStat
id="word-count"
className="stat-display__value"
stat={statistics.word_count}
statMsg={I18n.t('metrics.word_count')}
renderZero={true}
/>
<OverviewStat
id="references-count"
className="stat-display__value"
stat={statistics.references_count}
statMsg={I18n.t('metrics.references_count')}
renderZero={false}
/>
<OverviewStat
id="view-count"
className="stat-display__value"
stat={statistics.views_count}
statMsg={I18n.t('metrics.view_count_description')}
renderZero={true}
/>
</div>
);
};

NamespaceOverviewStats.propTypes = {
statistics: PropTypes.object.isRequired
};

export default NamespaceOverviewStats;
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import React from 'react';
import PropTypes from 'prop-types';

const OverviewStatsTab = ({ id, title, active, onClick }) => {
const isActive = (active) ? ' active' : '';
const tabClass = `tab${isActive}`;
return (
<div className={tabClass} onClick={onClick} id={id}>
<p>{title}</p>
</div>
);
};

OverviewStatsTab.propTypes = {
id: PropTypes.number.isRequired,
title: PropTypes.string.isRequired,
active: PropTypes.bool.isRequired,
onClick: PropTypes.func.isRequired
};

export default OverviewStatsTab;
99 changes: 99 additions & 0 deletions app/assets/javascripts/components/common/overview_stats_tabs.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import React, { useState } from 'react';
import PropTypes from 'prop-types';

import OverviewStatsTab from './OverviewStats/overview_stats_tab';
import NamespaceOverviewStats from './OverviewStats/namespace_overview_stats';
import WikidataOverviewStats from './wikidata_overview_stats';

import ArticleUtils from '../../utils/article_utils';


const OverviewStatsTabs = ({ statistics }) => {
const [currentTabId, setCurrentTabId] = useState(0);

const getTabTitle = (ns_id, wiki) => {
Copy link
Member

Choose a reason for hiding this comment

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

These two functions should probably be in a utils file rather than in this component. Tests for them would also be good.

const project = wiki.split('.')[1];
let ns_title = ArticleUtils.NamespaceIdMapping[ns_id];
if (typeof (ns_title) !== 'string') ns_title = ns_title[project];
return `${I18n.t(`namespace.${ns_title}`)} (${wiki})`;
};

const getContentTitle = (ns_id, wiki) => {
const project = wiki.split('.')[1];
let ns_title = ArticleUtils.NamespaceIdMapping[ns_id];
if (typeof (ns_title) !== 'string') ns_title = ns_title[project];
return `Stats for ${I18n.t(`namespace.${ns_title}`)} (${wiki})`;
Copy link
Member

Choose a reason for hiding this comment

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

This message shouldn't be hard-coded in English. Per the design image I shared, we might not need a message like this anyway; the label for each tab could just be the domain plus the namespace name (when there is one).

};

const onTabChange = (e) => {
return setCurrentTabId(Number(e.currentTarget.id));
};

const hasWikidataStats = () => {
if (statistics['www.wikidata.org']) return true;
return false;
};

const statsDataList = [];
// if there are wikidata overview stats, they should be displayed on first tab
Copy link
Member

Choose a reason for hiding this comment

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

This and the forEach loop are more data manipulation than I'd like to see. If possible, it would be best to do this in a way that doesn't special-case Wikidata. (I don't think it necessarily needs to be first.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I got this conceptual question. For each tab we have a different content rendered by different component. So, is it better to create this components, store and use them (like I am doing now), or is it better to initialize and render them instantaneously when a tab is selected?

I think in our case, the compoents are not that heavy and so they can probably be stored before hand.

if (hasWikidataStats) {
statsDataList.push({
id: 0,
tabTitle: 'Wikidata',
contentTitle: null, // title for wikidata stats already exists in WikidataOverviewStats component
data: statistics['www.wikidata.org']
});
}

let index = hasWikidataStats ? 1 : 0;
Object.keys(statistics).forEach((wiki_ns) => {
if (!wiki_ns.includes('namespace')) return; // return if it doesn't contain namespace stats

const ns_id = Number(wiki_ns.split('-')[2]); // namespace id
const wiki = wiki_ns.split('-')[0];
const id = index;
const tabTitle = getTabTitle(ns_id, wiki);
const contentTitle = getContentTitle(ns_id, wiki);
const data = statistics[wiki_ns];
statsDataList.push({ id, tabTitle, contentTitle, data });
index += 1;
});

const tabsList = statsDataList.map((obj) => {
return (
<OverviewStatsTab
key={obj.id}
id={obj.id}
onClick={onTabChange}
title={obj.tabTitle}
active={currentTabId === obj.id}
/>
);
});

const contentTitle = statsDataList[currentTabId].contentTitle;
const data = statsDataList[currentTabId].data;
const content = (hasWikidataStats && currentTabId === 0)
? <WikidataOverviewStats statistics={data} isCourseOverview={true}/>
Copy link
Member

Choose a reason for hiding this comment

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

It might make sense here to implement another simple component (eg, OverviewStatsTab (singular)), so that this tabs component has a simple job: render all the tabs and render the content for the current tab. The new component would be in charge of which component to render for the current tab content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I have already created a component named OverviewStatsTab which renders a tab.
May be we can name it OverviewStatsContent or OverviewStatsData or anything like that. And, you are saying that the logic of identifying the content (whether it is wikidata or namespace stats) according to active tab, should be done in this component? Instead of having statsList, we should use this new component?

Copy link
Member

Choose a reason for hiding this comment

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

You're right, OverviewStatsTab would be a bad name for it, and the component you have now called that is appropriately named. OverviewStatsContent sounds good.

And yes, I think having this component identify wikidata vs namespace stats should be done in that new component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ragesoss, I have created the OverviewStatsContent component. Please check it along with OverviewStatsTabs to confirm if that's okay.

Copy link
Member

Choose a reason for hiding this comment

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

yes, it looks good to me.

: <NamespaceOverviewStats statistics={data} />;

return (
<div className="overview-stats-tabs-container">
<div className="tabs-container">
{tabsList}
</div>
<div className="content-container">
<h2 className="title">{contentTitle}</h2>
<div className="stats-data">
{content}
</div>
</div>
</div>
);
};

OverviewStatsTabs.propTypes = {
statistics: PropTypes.object.isRequired
};

export default OverviewStatsTabs;
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ import PropTypes from 'prop-types';
import OverviewStat from './OverviewStats/overview_stat';
import I18n from 'i18n-js';

const WikidataOverviewStats = ({ statistics }) => {
const WikidataOverviewStats = ({ statistics, isCourseOverview }) => {
let containerClass = 'wikidata-stats-container';
if (isCourseOverview) containerClass = 'wikidata-stats-container course-overview';
return (
<div className="wikidata-stats-container">
<div className={containerClass}>
<h2 className="wikidata-stats-title">Wikidata stats</h2>
<div className="wikidata-display">
<div className="stat-display__row">
Expand Down Expand Up @@ -216,7 +218,8 @@ const WikidataOverviewStats = ({ statistics }) => {
};

WikidataOverviewStats.propTypes = {
statistics: PropTypes.object
statistics: PropTypes.object.isRequired,
isCourseOverview: PropTypes.bool
};

export default WikidataOverviewStats;
12 changes: 8 additions & 4 deletions app/assets/javascripts/components/overview/overview_handler.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { fetchOnboardingAlert } from '../../actions/course_alert_actions';
import { fetchTags } from '../../actions/tag_actions';
import { addValidation, setValid, setInvalid, activateValidations } from '../../actions/validation_actions';
import { getStudentUsers, getWeeksArray, getAllWeeksArray, firstValidationErrorMessage, isValid } from '../../selectors';
import WikidataOverviewStats from '../common/wikidata_overview_stats';
import OverviewStatsTabs from '../common/overview_stats_tabs';

const Overview = createReactClass({
displayName: 'Overview',
Expand Down Expand Up @@ -164,13 +164,17 @@ const Overview = createReactClass({
) : (
<div className="sidebar" />
);

let overviewStatsTabs;
if (course.course_stats && course.course_stats.stats_hash) {
overviewStatsTabs = <OverviewStatsTabs statistics={course.course_stats.stats_hash}/>;
}

return (
<section className="overview container">
{ syllabusUpload }
<OverviewStats course={course} />
{course.course_stats && <WikidataOverviewStats
statistics={course.course_stats.stats_hash['www.wikidata.org']}
/>}
{overviewStatsTabs}
<StatisticsUpdateInfo course={course} />
{userArticles}
<div className="primary">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,14 @@ import StudentRevisionRow from './StudentRevisionRow';

// Libraries
import CourseUtils from '~/app/assets/javascripts/utils/course_utils.js';
import ArticleUtils from '~/app/assets/javascripts/utils/article_utils.js';
import studentListKeys from '@components/students/shared/StudentList/student_list_keys.js';

export class StudentRevisionsList extends React.Component {
constructor(props) {
super(props);
this.state = {
isOpen: false,
namespace: 'all'
namespace: -1 // show all namespace revisions by default
Copy link
Member

Choose a reason for hiding this comment

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

For a code comprehensibility perspective, I don't think replacing strings with numbers here is helpful. Do the changes here have any effect on behavior? If they are unrelated to displaying stats on the Overview tab, they probably shouldn't be included in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes are related to the namespace filter of revisions table.
Actually, earlier this code used namespaceToId function from ArticleUtils. But, now that I have defined new mapping function there (NamespaceIdMapping), that function would have become its duplicate. Hence, some changes reflected here.
Should I open another pr then?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is okay, except... is there any reason to use -1 in place of all? The other integer values all map to a mainspace, but this one does not and -1 not as easy to interpret as all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I don't know what number to use for 'all', because for other options it would be their namespace id. May be, we can still use the strings - 'all', 'article', 'talk', etc.

Copy link
Member

Choose a reason for hiding this comment

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

I think a mix of numbers (for actual namespaces) and a string for all would be okay. But also, using strings for all of the values seems find to me.

};

this.toggleDrawer = this.toggleDrawer.bind(this);
Expand All @@ -25,7 +24,7 @@ export class StudentRevisionsList extends React.Component {
onNamespaceChange = (e) => {
// Open the drawer when filter is used
if (!this.state.isOpen) this.setState({ isOpen: true });
return this.setState({ namespace: e.target.value });
return this.setState({ namespace: Number(e.target.value) });
};

// filter the revisions according to namespace
Expand All @@ -34,10 +33,10 @@ export class StudentRevisionsList extends React.Component {

let revisions = [];
if (userRevisions[student.id] !== undefined && userRevisions[student.id] !== null) {
revisions = (this.state.namespace === 'all')
revisions = (this.state.namespace === -1)
? userRevisions[student.id]
: userRevisions[student.id].filter((rev) => {
return rev.article.namespace === ArticleUtils.namespaceToId(this.state.namespace);
return rev.article.namespace === this.state.namespace;
});
}
return revisions;
Expand Down Expand Up @@ -98,10 +97,10 @@ export class StudentRevisionsList extends React.Component {
value={this.state.namespace}
onChange={this.onNamespaceChange}
>
<option value="all">{I18n.t('namespace.filter.all_ns')}</option>
<option value="article">{I18n.t('namespace.filter.article_ns')}</option>
<option value="user">{I18n.t('namespace.filter.user_ns')}</option>
<option value="talk">{I18n.t('namespace.filter.talk_ns')}</option>
<option value={-1}>{I18n.t('namespace.all')}</option>
<option value={0}>{I18n.t('namespace.article')}</option>
<option value={2}>{I18n.t('namespace.user')}</option>
<option value={1}>{I18n.t('namespace.talk')}</option>
</select>
);
return (
Expand Down
3 changes: 2 additions & 1 deletion app/assets/javascripts/reducers/course.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ const initialState = {
loading: true,
updates: {},
wikis: [],
article_count: 0
article_count: 0,
course_stats: {}
};


Expand Down
46 changes: 37 additions & 9 deletions app/assets/javascripts/utils/article_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,41 @@ export default class ArticleUtils {
return project === 'wikidata' ? `${messageKey}_wikidata` : messageKey;
}

// Mapping of namespace to its id
static namespaceToId(namespace) {
const mapping = {
article: 0,
talk: 1,
user: 2
};
return mapping[namespace];
}
// The following mapping is also present in article.rb file and,
// wiki.rb has mapping of wikis to namespaces. Hence, any modifications
// in namespaces should also reflect in the corresponding files.
static NamespaceIdMapping = {
0: 'main',
1: 'talk',
2: 'user',
4: 'project',
6: 'file',
8: 'mediaWiki',
10: 'template',
12: 'help',
14: 'category',
104: 'page',
108: 'book',
110: 'wikijunior',
114: 'translation',
118: 'draft',
120: 'property',
122: 'query',
146: 'lexeme',
100: {
wiktionary: 'appendix',
wikisource: 'portal',
wikiversity: 'school'
},
102: {
wikisource: 'author',
wikibooks: 'cookbook',
wikiversity: 'portal'
},
106: {
wiktionary: 'rhymes',
wikisource: 'index',
wikiversity: 'collection'
}
};
}
Loading